Hot Topics
Security Bloggers Network 
SBN

AIRA Lab Robonomics Smart Contracts Audit

by Arseny Reutov on October 10, 2018

Positive.com experts have assessed the source code of AIRA Lab Robonomics smart contracts.

The final version of the audited contracts can be found in the Airalab/robonomics_contracts Github repository, branch master. The version used for the initial testing is the commit d4be12. The files Ambix.sol and DutchAuction.sol were audited based on the commit a867de.

The goal of the audit is to find vulnerabilities, logical flaws, and other code errors.

Summary

One (1) critical, two (2) high, two (2) medium and three (3) low severity issues were identified. Best practices aiming to achieve a higher quality of code were recommended.

A re-entrancy attack was identified in which adversaries would exploit the vulnerable contract to mint XRT tokens.

A compliance issue identified in the XRT.sol contract could impact the usability of the token.

The Congress.sol contract is compiled with an old compiler version of Solidity. Even though an immediate threat was not identified against this contract, it is advised to develop a migration plan for future use.

All critical, high and medium issues have been mitigated. One (1) low issue remains OPEN.

Critical Severity

Re-entrancy attack on RobotLiabilityLib:finalize()

The function RobotLiabilityLib:finalize() is susceptible to a re-entrancy attack. This issue is an attack performed by a lighthouse against the Robonomics platform. This attack may take place if a liability is created with a malicious token contract instead of an ERC20 token contract.

The entry point of this attack may be found in lines 126 or 128 of the RobotLiabilityLib contract where a call to an untrusted contract is being performed. Once the re-entrancy happens, the adversaries are seeking to trigger multiple times the require() statement in line 137.

Snippet from the RobotLiabilitLib:finalize() function showing the entry points and the targets of the reentrancy attacks

By populating the malicious transfer() function with a call to RobotLiabilityLib.sol:finalize() it is possible to trigger the function LiabilityFactory.sol:liabilityFinalized() multiple times.

Malicious transfer() function calling the RobotLiabilityLib:finalize() function

With this tactic, the adversary will be able to mint more XRT tokens that it is supposed to. The attack is demonstrated in the exploit contract below.

Goal of the re-entrancy attack is to trigger multiple times the xrt.mint() function

The flag isFinalized, which is currently used, is not enough to mitigate this attack. It is advised to disallow all scenarios where a contract function is calling itself, directly or indirectly. A ready-made solution for this type of security issues is ReentrancyGuard.sol, a contract developed by Zeppelin.

As per 2e9fef, isFinalized is now set to true before an external contract is called.

High Severity

Computational bias on DutchAuction:calcTokenPrice()

The function DutchAuction:calcTokenPrice() calculates the token price with 18 decimals instead of 9. All of the other functions of the contract make use of 9 decimals as it is declared in XRT.sol@L9.

The current implementation of token price calculation will directly negatively affect the number of tokens transferred to the Ambix contract at DutchAuction:finalizeAuction()@L247–248.

It is advised to ensure that all operation involving the decimals of the XRT token are consistent in using the same value, e.g., 9 or 18.

soldTokens will have an invalid value in case it is calculated with calcTokenPrice()

Decimal inconsistency posed a threat only in commit d4be12 and was fixed in commit a867de and later which was used for the audit. As such this issue is considered invalid.

Missing access control on LiabilityFactory:setENS()

The function LiabilityFactory:setENS() is not performing any authentication or authorization checks. Lack of those measures allows for the unauthorized and unauthenticated use of this function. Currently, it is possible for an adversary to trigger this function and set a malicious ENS resolver (L54). It must be noted that this function can only be triggered once.

It is advised to ensure that only authorized users can trigger this function.

The function setENS() is lacking authentication and authorization checks

As per 2e9fef, affected functionality setENS() has now been removed.

Medium Severity

XRT token ERC20 compliance violation

The XRT.sol token contract declares the decimals variable as a uint type. By default, uint is a uint256 variable type which is in direct violation of ERC20 compliance.

This issue may have a negative impact on the token’s future use and utility.

ERC20 compliant tokens should declare their decimals variable as a uint8 variable type.

Snippet from XRT.sol depicting the use of uint variable type for the decimals variable

As per 2e9fef, decimals are now declared as a uint8 variable.

Missing input validation on Lighthouse:_minimalFreeze variable

The constructor function of the Lighthouse.sol contract is missing a validation for _minimalFreeze at line 15, allowing for 0 to be a possible value. In case a Lighthouse is created with 0 as the value for _minimalFreeze then the function LighthouseAPI:quotaOf() will always lead to division by zero which, in turn, will block the process of creating liabilities.

It is advised to enforce a validation check ensuring that _minimalFreeze cannot be assigned the value of 0.

Use of _minimalFreeze on LiabilityFactory.sol without validation.
Use of _minimalFreeze on the Lighthouse contract without validation.

As per 2e9fef, Lighthouse.sol:15 now verifies that _minimalFreeze is a positive value.

Low Severity

Missing input validation in DutchAuction:changeSettings()

The function DutchAuction:changeSettings() is not verifying that the input parameters _ceiling and _priceFactor are greater than 0.

The variables are used for mathematical operations which include multiplication and division. In case those variables are set to 0, the contract DutchAuction will start malfunctioning as it will perform division by 0.

It must be noted that input validation for these variables is present in the constructor of the contract and that the vulnerable function, DutchAuction:changeSettings() is protected by the DutchAuction:isWallet() and DutchAuction:atStage(Stages.AuctionSetUp) modifiers.

Reported variables are assigned without validation
Reported variables are assigned with validation in the constructor

As per 2e9fef, changeSettings() function has now been removed.

Deadlock in LightHouseLib:keepalive() modifier

A plausible scenario exists in the Lighthouse.sol:keepalive() function where its operation may never terminate regardless of how much gas the sender uses.

The function LighthouseLib.sol:keepalive() uses a while loop to iterate through the members array to identify in which position of the array the msg.sender is located. This loop will trigger the nextMember() function which will always produce a valid result no matter how many times it is called.

The implementation of nextMember() in combination with the while() loop of keepalive() creates a deadlock situation. The deadlock will be triggered if msg.sender() is not located inside the members array. As a result, the loop will run until all transaction gas is consumed.

In case the msg.sender is not located in the members array the loop will never break

It is advised to allow for the graceful termination of the transactions which trigger the keepalive modifier and where the msg.sender is not located in the members array.

Congress.sol compiled with old Solidity version

The Congress.sol contract is compiled with a very old version of Solidity. The used version for this contract 0.4.9 (the contract declared version ^0.4.4 and was compiled with version 0.4.9), has at least one known issue that might directly affect this contract.

The 0.4.9 version of the solidity compiler is vulnerable to the SkipEmptyStringLiteral issue. In case a function is called with a string literal and the value “”, the encoder will skip it. This compiler quirk will cause the corruption of the function call data as the encoding of all arguments will be shifted 32 bytes to the left.

This issue can be mitigated by migrating the contract and compiling it with a newer compiler version.

Snippet from Etherscan reporting the compiler version used for this contract. Full page at https://etherscan.io/address/0x97282A7a15f9bEaDC854E8793AAe43B089F14b4e#code

Notes & Additional Information

This section includes best practice information and various recommendations which if followed will increase the quality of code. Issues listed in this section do not pose a security threat. The list reflects retesting status as per commit 2e9fef.

Ambix.sol

  • Consider using a fixed compiler version (L1) — OPEN
  • Consider using the delete statement for entries of the A and N array (L57, L58) — FIXED

DutchAuction.sol

  • Consider setting the visibility modifier first (L223, L233) — FIXED

LiabilityFactory.sol

  • The contract is making use of the variable tx.origin (L160, L228). Even though our assessment did not reveal any threat related to the verb tx.origin it is advised to consider removing it. The use of tx.origin is dangerous and may lead to future vulnerabilities — OPEN
  • Consider using a fixed compiler version (L1) — OPEN
  • Consider explicitly specifying constant visibility (L47) — FIXED
  • Consider placing internal functions after external ones for better readability of the contract — OPEN

LightContract.sol

  • Consider using a fixed compiler version (L1) — OPEN
  • Consider explicitly specifying variable visibility (L7) — OPEN
  • The constructor function is lacking a sanity check for the _library variable. It is possible to create a LightContract with an empty value for the _library parameter — OPEN

LightHouse.sol

  • Consider using a fixed compiler version (L1) — OPEN

LightHouseABI.sol

  • Consider using a fixed compiler version (L1) — OPEN

LightHouseAPI.sol

  • Consider using a fixed compiler version (L1) — OPEN
  • Consider explicitly specifying variable visibility (L8) — OPEN
  • Consider adding a security check at the function quotaOf(). Currently, the function lacks a sanity check for the _member variable. The function should verify that the value of the _member variable is not empty — OPEN

LightHouseLib.sol

  • Consider using a fixed compiler version (L1) — OPEN
  • Consider placing internal functions after external ones for better readability of the contract — OPEN
  • Consider using the delete operation to remove entries of the members array — OPEN
  • Consider adding a sanity check to function to() verifying that the _to address is not 0 — FIXED

RobotLiability.sol

  • Consider using a fixed compiler version (L1) — OPEN

RobotLiabilityABI.sol

  • Consider using a fixed compiler version (L1) — OPEN

RobotLiabilityAPI.sol

  • Consider using a fixed compiler version (L1) — OPEN

RobotLiabilityLib.sol

  • Consider using a fixed compiler version (L1) — OPEN

XRT.sol

  • Consider using a fixed compiler version (L1) — OPEN

Congress.sol

  • The current in-use Congress contract is an old version of the congress contract by Ethereum Foundation. An updated version of the DAO-congress contract (for Solidity 0.4.16) can be found in https://github.com/ethereum/ethereum-org/blob/master/solidity/dao-congress.sol — N/A
  • Consider using a fixed compiler version (L1) — N/A
  • Consider explicitly specifying function visibility. By default, function visibility is set to public — N/A

AIRA Lab Robonomics Smart Contracts Audit was originally published in ICO Security on Medium, where people are continuing the conversation by highlighting and responding to this story.

*** This is a Security Bloggers Network syndicated blog from ICO Security - Medium authored by Arseny Reutov. Read the original post at: https://blog.positive.com/aira-lab-robonomics-smart-contracts-audit-70954393ed05?source=rss----820ff037acec---4

Arseny Reutov , ,