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 f24ea6c5787b2d40423f4dc312d832592b1cd335 at branch master.
Summary
The audited contracts are:
- MeshToken.sol: Token implementation.
- MeshCrowdsale.sol: Crowdsale implementation.
- Migrations.sol: Truffle migration contract.
These checks are performed:
- isuse of the several 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 override 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 pause the token twice:
function pause() onlyOwner whenNotPaused public {
require(pausedOnce == false);
pausedOnce = true;
super.pause();
}
In order to stop depending on the behavior of allowedTransfers, you may also remove whenNotPause modifier from pause and move the requirement inside the function. Like this:
function pause() onlyOwner public {
require(!paused && pausedOnce == false);
pausedOnce = true;
super.pause();
}
Update: The RightMesh team declared they do not want the Owner to make token transactions.
Function setLimit data race softlock
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
require(_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 cause the function to fail. Without even considering a hypothetical attack, this function may get softlocked indefinitely by multiple people contributing.
This can be fixed by directly setting the maximum value between the current weiContribution for the address and the new weiLimit. For example, assuming max is already implemented:
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];
weiLimits[_address] = max(_weiLimit, weiContributions[_address]);
}
return true;
}
Update: The RightMesh team declared they will never use this function to reduce weiLimits, only to increase them which avoids the issue.
Outdated and differing solidity pragma versions
The three files are using different old Solidity versions. Migrations.sol and MeshToken.sol are using 0.4.17, while MeshCrowdsale.sol is using 0.4.15. Consider updating them all to the same latest version of Solidity.
Update: This was fixed in commit 59c1977301c071e3cd7fac670b00aa51d87bf1aa
Enhancements
Remove unnecessary pause and unpause functions from Crowdsale
The following functions in the crowdsale are not needed:
/**
* @dev Allows the contract owner to pause the token transfers on deployed token
*/
function pauseToken() external onlyOwner {
MeshToken(token).pause();
}
/**
* @dev Allows the contract owner to unpause the token transfers on deployed token
*/
function unpauseToken() external onlyOwner {
MeshToken(token).unpause();
}
These functions are made redundant by transferTokenOwnership:
/**
* @dev Allows the current owner to transfer token control back to contract owner
*/
function transferTokenOwnership() external onlyOwner {
token.transferOwnership(owner);
}
Which allows the owner to call the pause and unpause functions in the token directly.
Update: Fixed in commit c5454aa92583b21273764cc22ae595209b46c92e
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 feasible. Neither contract added overcomplicated or large elements to the implementations which is also a desirable approach.
Do you want to know what is Coinfabrik Auditing Process?
Check our A-Z Smart Contract Audit Guide or you could request a quote for your project.
