The WeTrust team asked us to review and audit their new ROSCA contract code. We looked at their contracts and now publish our results.

The audited contracts are at their rosca-contracts GitHub repo. The version used for this report is commit 2af29be97d529488f5488fe0592f9e6b3585254f. The main contract file is ROSCA.sol.

Here’s our assessment and recommendations, in order of importance:

Severe

We haven’t found any severe security problems with the code.

Potential problems

Use safe math

There are many unchecked math operations in the code. It’s always better to be safe and perform checked operations. Consider using a safe math library, or performing pre-condition checks on any math operation.

Be careful with integer division

Several parts of the code use integer division. See 1, 2, 3, 4, 5. Integer division result will discard the remainder and keep the quotient. This could bring unexpected results in some cases.

For example, in line 288, if currentRoundTotalDiscounts is not a multiple of membersAddresses.length, totalDiscounts will be incremented by the quotient, and the remainder of the division will not be considered.

We haven’t detected any actual attacks or inconsistencies in the contract due to this fact, but we recommend being extra careful. In this case, a solution could be to make totalDiscounts have the total amount of discounts instead of the discounts per member.

Timestamp usage

There’s a problem with using timestamps and now (alias for block.timestamp) for contract logic, based on the fact that miners can perform some manipulation. In general, it’s better not to rely on timestamps for contract logic. The solutions is to use block.number instead, and approximate dates with expected block heights and time periods with expected block amounts.

The ROSCA.sol contract uses timestamps at several points. The risk of miner manipulation, though, is really low. The potential damage is also limited: miners could only slightly manipulate when each round starts and who the winner is in each round. As this comment notes, this won’t affect the functioning of the contract, but the miner of the cleanUpPreviousRound call transaction will have absolute control on who the next winner is. We recommend the team to consider the potential risk of this manipulation and switch to block.number if necessary.

For more info on this topic, see this stack exchange question.

Warnings

Use of send

Use of send is always risky and should be analyzed in detail. Three occurrences found in line 427, line 476, and line 496 of ROSCA.sol.

- Always check send return value: OK.

- Consider calling send at the end of the function: OK.

- Favor pull payments over push payments: Warning. All 3 occurrences of send are push payments. Although we couldn’t find any attack vectors on this contract, consider using OpenZeppelin’s PullPayment contract to implement pull payments in ROSCA.sol.

For more info on this problem, see this note.

Usage of magic constants

There are several magic constants in the contract code. Some examples are:

Use of magic constants reduces code readability and makes it harder to understand code intention. We recommend extracting magic constants into contract constants.

Bug Bounty

Formal security audits are not enough to be safe. We recommend implementing an automated contract-based bug bounty and setting a period of time where security researchers from around the globe can try to break the contract’s invariants. For more info on how to implement automated bug bounties with OpenZeppelin, see this guide.

totalFees will always be 0 on LogFundsWithdrawal

In line 502 of ROSCA.sol, a LogFundsWithdrawal event is emitted if the send does not fail. The problem is that the variable totalFees will always be 0, as it is set to 0 in line 495.

Avoid duplicated code

Duplicate code makes it harder to understand the code’s intention and thus, auditing the code correctly. It also increases the risk of introducing hidden bugs when modifying one of the copies of some code and not the others.

The logic in getParticipantBalance() and the start of withdraw() is very similar and could be refactored to avoid repetition. Consider using getParticipantBalance in withdraw.

Naming suggestions

The members_ constructor parameter could be confused as the full set of members, and it is in fact al members but the contract creator. Consider calling it otherMembers_ or something like so, to avoid this confusion.

constructor parameter could be confused as the full set of members, and it is in fact al members but the contract creator. Consider calling it or something like so, to avoid this confusion. The grossTotalFees uint256 variable in recalculateTotalFees is not really the gross total fees, because it’s never multiplied by the service fee per mille. Consider renaming it to grossTotal or something like so, to avoid confusion.

uint256 variable in recalculateTotalFees is not really the gross total fees, because it’s never multiplied by the service fee per mille. Consider renaming it to or something like so, to avoid confusion. The totalDiscounts uint256 contract variable does not represent the total discounts, but the discounts per member. Consider renaming to discountPerMember to avoid confusion.

Use latest version of Solidity

Current code is written for an old version of solc (0.4.4). We recommend changing the solidity version pragma for the latest version ( pragma solidity ^0.4.10; ) to enforce latest compiler version to be used.

Additional Information and Notes

Conclusions

No severe security issues were found. Some changes were recommended to follow best practices and reduce potential attack surface.

Overall, code quality is good, it’s well commented, and most well-known security good practices were followed. This was one of the most well written contracts we had to audit. 👍

If you’re interested in discussing smart contract security, follow us on Medium or join our slack channel. We’re also available for smart contract security development and auditing work.