The Coinfix team asked us to review and audit their MerchantSubscription contract. We looked at the code and now publish our results.
The audited code is located in the ChainXcom/coinfix.io-smart-contracts repository. The version used for this report is commit 75edd659ce96c88fe2784f3e767d6617787e8b7c.
Here is our assessment and recommendations, in order of importance.
Update: The Coinfix team has followed most of our recommendations and updated the contracts. The new version is at commit c02c083173fee08fb6e8e20ff854e5fbbbe9ed55.
No issues of critical severity.
No issues of high severity.
A MerchantSubscription contract can be instantiated with the null address as the merchant parameter. Such an instance will never be able to be activated or function at all. Consider adding a sanity check in the constructor to require that the merchant parameter is different from 0x0. This will also be in line with the principle of failing as early and loudly as possible.
Update: Fixed in this commit by checking merchant != 0x0.
The contracts can be compiled with versions of the compiler as low as 0.4.11. We recommend changing the Solidity version pragma to a more recent version (for example, pragma solidity ^0.4.17) to enforce the use of an up to date compiler.
Update: Fixed in this commit.
The contract implements similar functionality to code found in OpenZeppelin’s SafeMath,Ownable (or
Claimable) and Pausable. Reimplementing functionality instead of reusing public and already audited code can bring regression problems and difficult to find bugs. Consider removing the duplicate code from your repo and using the installed versions from OpenZeppelin.
Update: Fixed in this commit by using OpenZeppelin’s contracts.
The contract has an amount state variable, which intends to track the amount of ether received. However, it is redundant because such a value is already accessible via this.balance.
Additionally, bear in mind that the contract can receive ether bypassing the fallback function (see the second warning here. This would leave the amount variable out of sync with the actual balance, and the ether received in such a way unrecoverable. Moreover, the SubscriptionPaymentMade event will not be emitted for these cases.
Consider removing this variable from the contract, and using this.balance instead. After this removal SafeMath will be unnecessary.
Update: Fixed in this commit by using this.balance.
constructor, fallback function nor the claimOwnership method have explicit visibility. Solidity defaults to public. Consider making this explicit. This will also avoid some compiler warnings introduced in the latest versions, motivated by the first Parity wallet hack.version state variable could be declared constant.SafeMath methods could be marked pure. Consider using Solidity’s using for feature for enhanced readability.SafeMath.)SubscriptionPaymentMade’s customer as it enables faster lookups for consuming apps.withdrawal function transfers ether to the merchant address (by using Solidity’s transfer). Make sure that merchant addresses can receive ether.No critical or high severity issues were found. Some changes were proposed to follow best practices and reduce potential attack surface.
Note that as of the date of publishing, the above review reflects the current understanding of known security patterns as they relate to Coinfix’s contract. We have not reviewed the related Coinfix project. The above should not be construed as investment advice. For general information about smart contract security, check out our thoughts here.