Money on Chain Security Audit III

This is the third of a series of four audits we performed for MOC: Previous Audit, Next Audit

Introduction

CoinFabrik was asked to audit the contracts for the Money On Chain project. We will provide an executive summary of our discoveries, a short description of the project, the methodology used, the details of our findings and will finish with our conclusion of the code audited.

Executive Summary

This is the second audit we perform on the code of the Money On Chain project. 

We didn’t found issues with critical or high risk.

We found one issue with medium risk since it involves the contracts initialization. From our tests it cannot be exploited. We notified the team so they document it properly or make proper fixes.

There are a few minor issues/enhancements that do not affect the contract functionality, for example contracts variable with helpful names, or some error conditions which will not generate a message. 

Contracts

The contracts audited are from the Money on Chain project. The audited contracts consist of 3 distinct functionalities:

• Money On Chain: Money On Chain is a suite of smart contracts dedicated to providing a bitcoin-collateralized stable-coin.
• Governance: A suite of smart contracts dedicated to providing a governance system which is generic enough to work without knowing the system to be governed.
• Oracle: Money on Chain USD-BTC price provider.

The following dependency graph shows the most important contracts from the Money On Chain repository:

Analyses performed

The following analyses were 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 anything other than them being strictly increasing.
• Contract softlocking attacks (DoS).
• Potential gas cost of functions being over the gas limit.
• Missing function qualifiers and their misuse.
• Fallback functions with a higher gas cost than the one that a transfer or send call allows.
• Fraudulent or erroneous code.
• Code and contract interaction complexity.
• Wrong or missing error handling.
• Potential overuse of transfers in a transaction might produce unnecessary fees. Withdrawal pattern should be used instead.
• Insufficient analysis of the function input requirements.

Detailed findings

Medium severity

Usage of array deletes may surpass gas limits

Array deletes have a linear cost even when they reimburse gas. Gas reimburse is applied after the gas limit checks are done. Therefore even if the final gas calculation lies below the gas limit the transaction may still fail if it surpassed the gas limit at some point.

For this reason a delete operation can fail due to gas costs if the array is big enough leading to DoS. There are two array deletes. One in MoCSettlement:

function clear() public onlyWhitelisted(msg.sender) {
    delete redeemQueue;
}

and one in MoCBucketContainer:

function clearBucketBalances(bytes32 bucketName) public onlyWhitelisted(msg.sender) {
    MoCBucket storage bucket = mocBuckets[bucketName];
    bucket.nBPro = 0;
    delete bucket.activeBalances;
}

We recommend revising the gas cost of these operations. You may need to add paging to these operations.

Minor severity

Conflicts on secure contract initialization

The project uses ZeppelinOS to allow upgrades without having to migrate contract’s data. To achieve this it has inherit from Initializable from ZeppelinOS which provides a modifier initializer for secure initialization of upgradeable contracts.

But it also inherits from MoCBase, which provides a similar modifier onInitialization to protect the contract against unsecure initialization. 

This is an issue, Initializable assumes the first function to be called will be modified by initializer to ensure that only that function will set initialized to true after returning. But this is not what happens, as the first function is onInitialization which later initializes Stoppable which is Initializable. if another contract using Initializable was inherited and initialized by this function, it will fail as Stoppable already set initialized to true.

We recommend only using the ZeppelinOS version to avoid these types of issues in the future.

Storage variable with the same name

The MoC contract inherits from Initializable and MoCBase. They use a variable with the same name initialized, it is private in ZeppelinOS but it is internal in MoCBase which allows access from derived contracts. Since solidity allows multiple inheritance and uses C3 linearization to determine the precedence order it is possible that a change in the inheritance order in a derived contract will affect the variable being referenced.

We suggest to always declare variables with the least possible scope, since initialized is not used outside of MoCBase is better to declare it as private.

Old solidity version

The contracts for the Oracle project require solidity version 0.4.24 which was released on May 2018. While we didn’t find any vulnerability related to using this specific version, we recommend upgrading to a more recent version as many issues and ambiguities get fixed in each release. If contracts can’t be upgraded to v0.5 you should consider using v0.4.26 the latest version of the v0.4 branch.

For example in oracle\contracts\price-feed\price-feed.sol we have:

    pragma solidity ^0.4.23;

Unhelpful variables names

There are a few instances in the Oracle contracts that variables and parameters have names that are not helpful to understand the code. 

For example function read() in lib/value.sol

    function read() public view returns (bytes32) {
        bytes32 wut; bool haz;
        (wut, haz) = peek();
        require(haz, "haz-not");
        return wut;
    }

Missing message in requires

The require() statement has an optional message parameter that will return in case of failure of the testing condition.

• oracle/contracts/lib/math.sol: the function add(), sub() and mul() have a require without message

function add(uint x, uint y) internal pure returns (uint z) {
        require((z = x + y) >= x);
    }

Observations/Remarks

Upgradability

A major change introduced to Money On Chain in the second audit was to make contracts upgradeable using ZeppelinOS. 

One important benefit of this feature is that in case of bugs the contracts can be upgraded without requiring changes to third party tools or intervention from users. Also contracts data doesn’t have to be migrated lowering costs.

This feature has the drawback that there is one special account that control the upgrade. It is possible for the entity controlling this account to upgrade the contracts to a completely different version without approval from users.

There is also a new feature in the contracts called “changers” that have similar but reduced implications. It allows the owner to grant permission to an arbitrary contract, the changer, for a single transaction to make changes to storage variables inside the project. That is, it allows the owner to modify multiple selected variables on different contracts in a single transaction, instead of multiple ones which may cause issues, by delegating the task to another deployed contract. Since the whitelisting is decided by the owner at the moment of the transaction it can be used to execute other changers that are not included in this audit. This is not considered a vulnerability since the variables that changers are allowed to mutate are pre-selected and it’s similar to having other privileged functions that can alter parameters. It does however, increase the surface area for errors if not handled with care.

Conclusion

We consider the contracts to be well written and abundantly documented, they use reasonable recent version of popular frameworks like OpenZeppelin, ZeppelinOS and most code use solidity compiler version 0.5.

We found a medium severity issue that is not exploitable regarding contracts initialization. A few minor issues that do not affect functionality and are about code style, using an old compiler version.

We also add the observation that upgradability can be considered a feature but it can also be considered a bug because it allows the owner to arbitrarily change the deployed bytecode.

Disclaimer: This audit report is not a security warranty, investment advice, or an approval of the Money On Chain project since CoinFabrik has not reviewed its platform. Moreover, it does not provide a smart contract code faultlessness guarantee.

An exhaustive report of the audit along with its reviews can be found here.