The WeTrust team asked us to review and audit their new ROSCA contract code. We looked at their contracts and now publish our results.
Here’s our assessment and recommendations, in order of importance:
We haven’t found any severe security problems with the code.
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
For example, in line 288, if currentRoundTotalDiscounts is not a multiple of membersAddresses.length, totalDiscountswill 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.
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 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
Use of send
- 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
Use of magic constants reduces code readability and makes it harder to understand code intention. We recommend extracting magic constants into contract constants.
totalFees will always be
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 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.
- 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.
- 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
- Good naming for logs, all starting with
Log, as recommended in our article about smart contract security best practices
- Good work with using a fail-early-and-loudly programming style. Most exceptional conditions are handled with
throws, as recommended in our guide. For example, all modifiers do this.
- Good work using the EscapeHatch pattern to make the contract safer.
- FEE_ADDRESS uses a different naming convention to other constants. Consider regularizing notations.
- Bad indentation at line 243.
- Consider checking against
0when comparing addresses. E.g:here and here
!winnerSelectedThroughBidin the conditional in line 253
- Consider changing these complex condition checks into multiple ifs and throws.
- Interesting error handling technique in line 394
- No need to throw in line 430, but still simpler and equivalent to do so.
- Comment in line 444 contains a typo, should read: “if ROSCA has ended”.
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. 👍
Note that as of the date of publishing, the above review reflects the current understanding of known security patterns as they relate to the ROSCA contract. We have not reviewed the related WeTrust project. The above should not be construed as investment advice or an offering of tokens. For general information about smart contract security, check out our thoughts here.