AIRA Lab Robonomics Smart Contracts Audit
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.
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.
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.
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 [email protected]
.
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.
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.
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.
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.
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.
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.
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.
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 theA
andN
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 verbtx.origin
it is advised to consider removing it. The use oftx.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