RIF Token Smart Contract Audit

Executive Summary

Introduction

Before the token starts working as such there is an initialization phase where the token is set up, IOVLabs and shareholders are awarded a number of tokens which are stored in individual lockup contracts which control how these are distributed in stages as the different milestones are reached. Additionally, contributors may be added which receive the tokens upon distribution, but are encouraged to keep them by a bonus payout divided in different time lapses. All of these tasks are performed by a contract called TokenManager, which is specifically authorized to perform such actions in the RIFToken contract.

Contributors must choose where should their tokens and bonuses be distributed at the time of redeeming them. They can simply specify that their tokens should be left on their original address, or they can choose to have the tokens redirected to a different address by sending a specially crafted message signed with their private key which specifies the new address. A third redeem method is available which can only be called by the RIFToken contract owner, which uses a message with the text “DELEGATION” signed by the original contributor public key. This message must be generated by the contributor in advance and kept safely stored in case they lose access to their original account.

After the distribution is over, the contract works as a regular ERC20 token, which also implements the ERC677 transferAndCall as a backward compatible enhancement of ERC20.

Audit Timeline

In October 2018, after the identified issues were addressed by the IOVLabs team, a new security audit of the contracts was performed. The fixes for the previous issues were verified and two new low risk issues and one medium issue were identified.

The initial audit included the rc-1 tag of a private Git repository up to commit e7d81ef93256a613faef540df9bffb5aac396b74, comprising the following Solidity files with their respective SHA-256 hash:

The second audit included the rc-2 tag of a private Git repository up to commit b045729bb556fd02399ca5ca3ddc247b5e35d908, comprising the following Solidity files with their respective SHA-256 hash:

Coinspect verified that all the identified issues were fixed in the rc-3 tag of a private Git repository up to commit 6194d7edca0abbcb5275350da7b225edd18b7573, comprising the following Solidity files with their respective SHA-256 hash:

The content of the files Contributors[1–5].sol with addresses and balance of contributors was not reviewed.

Summary of Findings

Findings

Description

Contributors are not supposed to be able to move their tokens before they redeem them. But function transferFrom in RIFToken doesn’t enforce this:

function transferFrom(address _from, address _to, uint256 _value) public returns (bool) {
bool result = super.transferFrom(_from, _to, _value);
if (!result) return false;

doTrackMinimums(_from);

return true;
}

And RIFToken inherits function approve from StandardToken.sol:

function approve(address _spender, uint256 _value) public returns (bool) {
allowed[msg.sender][_spender] = _value;
emit Approval(msg.sender, _spender, _value);
return true;
}

A contributor could take advantage of this vulnerability to move funds before redeeming them, by calling approve with a destination address and then calling transferFrom to move token to the destination address.

There were already tests in place for approve and transferFrom to make sure this is not possible, but the tests were broken.

Recommendations

Add checks in the approve function as well as increaseApproval and decreaseApproval to require that contributors calling the function had already redeemed the tokens.

It is advisable to add similar checks in the transferFrom function too, even though it should be unnecessary if the functions changing allowance already do the checks.

RIFToken inherits from StandardToken, and re-implements some functions in order to add controls. Special care must be taken to be sure that controls are added to all functions that need it, or it might be possible to bypass controls by using alternative inherited functions (such as approve and increaseApproval).

Additionally, check that the addresses specified in these functions are not ones that have been redirected to a new address, as in this case the funds may be lost.

Description

The contract TokenManager implements the function setShareholderAddress to assign a shareholder wallet address to an available token distribution. This function has no access controls, anyone could call it to steal tokens destined to shareholders.

Recommendations

Add the onlyOwner modifier to the function setShareholderAddress.

Description

The contract RIFToken implements two access control rules where some functions may only be called by the contract owner and some others may only be called by the authorizedManagerContract.

The contract owner is used to deploy and setup the token so that it works correctly, and one of the required tasks is to assign an authorizedManagerContract. The authorizedManagerContract is assigned to another contract which is used after the deploy to manage the different actions required by the contract such as transfer funds to contributors and shareholders, redirect funds from one user address to another and pay bonuses.

The contract owner is also capable of disabling the functions of the authorizedManagerContract by calling the disableManagerContract function. However, once this function is called once, a new manager contract can not be set. If this function is called before the manager contract completes all the tasks it is supposed to complete, some critical tasks such as bonus payments, unclaimed tokens recovery will never be completed.

Recommendations

Ensure the disableManagerContract function is not called before the auth orizedManagerContract completes all the tasks it is required to do.

Description

The contract RIFToken implements several functions to perform the redeem of the funds of a contributor. The function contingentRedeem allows the contract owner to assign an address different from the contributors’ original address as its redeem address.

When performing this function, the value of the variable minimumLeftFromSale which is used to calculate the bonus assigned to each contributor is changed. Furthermore, if a second contributor chooses to redeem to the same address, the value of said variable will be overwritten:

function contingentRedeem(

(…)

// Now we must move the funds from the old address to the new address

minimumLeftFromSale[redeemAddress] = minimumLeftFromSale[contributorAddress];

minimumLeftFromSale[contributorAddress] = 0;

(…)

Taking advantage of this, a contributor may cheat in order to get more bonus tokens. The steps to follow to achieve that are the following:

  1. Contributor A (with minimumLeftFromSale[A]=100 ) is redeemed to address Z:
    -> minimumLeftFromSale[Z] = minimumLeftFromSale[A] = 100
  2. Contributor B (with minimumLeftFromSale[B]=500 ) is redeemed to address Z:
    -> minimumLeftFromSale[Z] = minimumLeftFromSale[B] = 500
  3. payBonus is called in the TokenManager contract. Now the bonus for Contributor A is calculated and should be 100*bonus_percentage, but as this is calculated using minimumLeftFromSale[Z] which is now 500. The bonus will be of 500*bonus_percentage. Then the bonus for Contributor B is calculated, and the bonus is as expected 500*bonus_percentage.

Step 1 can be repeated N-times with different contributor addresses before performing Step 2 using the address with the highest contribution amount, in order to maximize the amount of extra tokens awarded to all of them. An attacker could create a smart contract to exploit this weakness and invite contributors to redeem to the attacker’s contract and share the benefit.

Recommendations

Either forbid one address to be used by two different contributions to redeem, or track the minimumLeftFromSale variable from the original contributor address in order to prevent problems at the time of calculating the bonus.

Description

The public method TransferAndCall in the RIFToken contract lacks visibility modifiers, such as public. It’s a good smart-contract programming practice to clearly distinguish between private and public methods, to prevent mistakes.

Recommendations

Add the “public” modifier to the public methods.

Description

The method redeemToSameAddress in the RIFToken contract specifies that a bool result will be returned, however there is no return statement in the function. As it is a public method, a contributor may include a call to that method within his own code expecting a bool result which will never be returned.

Recommendations

Add the “return True” statement to the function in case it runs correctly.

Description

The function AddressHelper.fromAsciiString accepts invalid hexadecimal strings and returns an address() with a 0 in the position of each invalid hexadecimal character. This function is used to obtain the destination address for the tokens when the RIFToken.redeem function is called by a contributor. When this redeem method is used, the contributor signs a message containing the destination RSK address encoded in hexadecimal and calls RIFToken.redeem passing the address as a string and the r,s and v values of the signature.

If the contributor makes a mistake and signs an address that is not even a valid hexadecimal string, the error is not detected by the contract and the funds are lost.

Recommendations

Revert the transaction when an invalid address string is passed to RIFToken.redeem.

Testing

The problems are some improper checks to assert that a function call throws an exception. For example in RIFTokenTransfer_test.js:

it(‘cannot transferFrom to a contributor’, async function () {
[…]
try {
await this.token.transferFrom(shareholderAccount, contributorAccount, 200, { from: anotherAccount });
assert.fail();
} catch (ex) {
}
});

This code is incorrect because assert.fail() works by throwing an exception that the test runner is expecting to catch to mark the test as failed, but the try/catch sequence included in the test code actually catches the exception and the test runner never gets it, so the test always passes.

Another problem with using a try/catch in this fashion is that it might hide other errors in the tests. For example in RIFToken_test.js:

it(‘cannot disable track minimum after release ownership’, async function () {
await this.token.releaseOwnership();
try {
await this.token.disableTrackMinimum();
assert.fail();
} catch (ex) {
}
});

The test above calls the function disableTrackMinimum, but that function doesn’t exist in the contract (the correct name is disableTrackMinimums), and this throws an error that is catched and ignored. Similarly, tests ‘only owner can disable redeem’ and ‘only owner can disable manager contract’ fail silently (because they call functions with an extra parameter that no longer exists in the contracts), but the problems are hidden by the try/catch.

In order to check function throws, it is appropriate to use expectThrow as in tests for LookupAccount and TokenManager, for example:

it(‘cannot transferFrom to a contributor’, async function () {
await this.token.transferToShareholder(shareholderAccount, 1000, { from: managerContract });
await this.token.transferToContributor(contributorAccount, 1000, { from: managerContract });
await this.token.approve(anotherAccount, 200, { from: shareholderAccount });
await expectThrow(this.token.transferFrom(shareholderAccount, contributorAccount, 200, { from: anotherAccount }));
});

It is recommended to increase test coverage. For example, in RIF-001 it was found that a contributor can use approve/transferFrom before redeeming the tokens. After fixing the try/catch problem, two existing tests find this issue. But the increaseApproval function is not tested at all, and it serves the same purpose as approve. Similarly, some tests about the behaviour of transfers don’t cover the several flavors of transfer and transferFrom.

The test “can recover no beneficiary shareholders” (included in TokenManager.tests.js) is not actually checking anything as no comparison or expected result is stated in order to verify the contracts are behaving correctly. We recommend completing this test.

Appendix

Disclaimer

Security for a Decentralized World

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store