Coinfabrik was asked to audit the contracts for the RightMesh Token sale. In the first part, we will give a summary of our discoveries and follow them with the details of our findings.
The contracts audited are from the RightMesh repository at https://github.com/firstcoincom/solidity. The audit is based on the commit 39027aadcb64551342fc39199af36ce0702dbdc2 at branch rightmesh, and updated to reflect changes at 39027aadcb64551342fc39199af36ce0702dbdc2.
Summary
The audited contracts are:
- MeshToken.sol: Token implementation.
- MeshCrowdsale.sol: Crowdsale implementation.
- Migrations.sol: Truffle migration contract.
These checks are performed:
- Misuse of the different call methods: call.value(), send() and transfer().
- Integer rounding errors, overflow, underflow and related usage of SafeMath functions.
- Old compiler version pragmas.
- Race conditions such as reentrancy attacks or front running.
- Misuse of block timestamps, assuming things other than them being strictly increasing.
- Contract softlocking attacks (DoS).
- Potential gas cost of functions being over the gas limit.
- Missing function qualifiers or misuse of them.
- Fallback functions with higher gas cost than what a transfer or send call allows.
- Underhanded or erroneous code.
- Code and contract interaction complexity.
- Wrong or missing error handling.
- Overuse of transfers in a single transaction instead of using withdrawal patterns.
- Insufficient coverage of function input requirements.
Detailed findings
Critical/Medium severity
None found
Minor severity
Owner cannot be allowed to transfer on pause
There is an overload of the whenNotPaused modifier which is used for all the token transfer related functions in OpenZeppelin implementation:
modifier whenNotPaused() {
require(!paused || allowedTransfers[msg.sender]);
_;
}
And allowedTransfers mapping can be modified by the owner, but not for himself:
function updateAllowedTransfers(address _address, bool _allowedTransfers)
external
onlyOwner
returns (bool)
{
// don't allow owner to change this for themselves
// otherwise whenNotPaused will not work as expected for owner,
// therefore prohibiting them from calling pause/unpause.
require(_address != owner);
allowedTransfers[_address] = _allowedTransfers;
return true;
}
We recommend removing this require, as the owner cannot really pause the token:
function pause() onlyOwner whenNotPaused public {}
And unpausing does not require the whenNotPaused modifier. In the worst case scenario, the owner may reset its status with updateAllowedTransfers again.
Function setLimit data race
function setLimit(address[] _addresses, uint256 _weiLimit) external returns (bool) {
require(whitelistingAgents[msg.sender] == true);
for (uint i = 0; i < _addresses.length; i++) {
address _address = _addresses[i];
// only allow changing the limit to be greater than current contribution
if(_weiLimit >= weiContributions[_address]) {
weiLimits[_address] = _weiLimit;
}
}
return true;
}
This function does not set the weiLimit for an address if said limit is bigger than its contributions. This can be exploited by buying tokens just before this function is called, to get over the limit which will make the function unable to set the limit for said address.
This can be fixed by directly setting the maximum value between the current weiContribution for the address and the new weiLimit.
Conclusion
The contracts were simple, they both relied on OpenZeppelin’s implementation of the token and sale. Using an already implemented, tested and audited token is always the best option if available. Neither contract added over complicated or large elements to the implementations which is also a desirable approach.
