Auditing a Core Smart Contract of the Colu Local Network
Recently, ProtoFire has reviewed the CLN Token smart contracts for the Colu’s team and submitted the results achieved. In this blog post, we deliver the results of auditing another smart contract for Colu — the Mechanical Market Maker, which lies at the core of the Colu Local Network.
Auditing scope of work
Under the audit, the ProtoFire team inspected the following contracts related to the Mechanical Market Maker in order to identify security issues, misbehavior, and any inefficiencies that might have occurred after the updates introduced.
- A smart contract for the Colu Community Currency.
- Interfaces for the general Market Maker and the Ellipse Market Maker.
- The Library contract for the Ellipse Market Maker implementation.
- The Delegation contract for the individually deployed Market Maker that will use the Library contract.
- Factory contracts for creating the Market Maker contract with a new token contract (the Colu Community Currency).
A list of smart contracts related to the Market Maker audit:
- ColuLocalCurrency.sol
- CurrencyFactory.sol
- EllipseMarketMaker.sol
- EllipseMarketMakerLib.sol
- IEllipseMarketMaker.sol
- IssueanceFactory.sol
- MarketMaker.sol
A list of the contracts to be reviewed:
- BasicToken.sol
- ColuLocalNetwork.sol
- ColuLocalNetworkSale.sol
- ERC20.sol
- Migrations.sol
- MultiSigWallet.sol
- MultiSigWalletWithDailyLimit.sol
- Ownable.sol
- SafeMath.sol
- TokenHolder.sol
- VestingTrustee.sol
The smart contracts are located in this GitHub repository. The version under audit is the [1a027f7f] commit.
The audit overview and good practices
It’s worth mentioning that the code is well-documented and formatted. The code structure is modular and carefully designed.
Additionally, the code encapsulates the SafeMath library — used in all math operations. No magic constants were found in the source code.
In total, the developer team wrote 1048 (!) tests, with most of them covering the functionality.
Update by the Colu team: Most of our previous recommendations were followed, and the contracts were updated accordingly.
Attacks against the contracts
We checked the contracts against known attacks, and no vulnerabilities were found.
Vulnerabilities found in the contracts by severity
1. Issues of critical severity
No critical-severity issues were found in the contracts within this scope.
2. Issues of high severity
No high-severity issues were found in the contracts within this scope.
3. Issues of medium severity
3.1 ColuLocalNetworkSale#initialize (ColuLocalNetworkSale.sol)
The transferTokens result was not checked.
// Issue the remaining tokens to designated pools.transferTokens(communityPoolAddress, COMMUNITY_POOL);
// stakeholdersPoolAddress will create its own vesting trusts.transferTokens(stakeholdersPoolAddress, STAKEHOLDERS_POOL);
Update by the Colu team: Fixed before deploying to the [4725790] commit, added require to the transferTokens methods.
3.2 ColuLocalNetworkSale#participate (ColuLocalNetworkSale.sol)
The transferTokens result was not checked.
transferTokens(_recipient, tokensToTransfer);Update by the Colu team: Fixed before deploying to the [4725790] commit, added require to the transferTokens methods.
3.3 VestingTrustee#unlockVestedTokens (VestingTrustee.sol)
The unlockVestedTokens(address _grantee) result was hidden in unlockVestedTokens(), so unlockVestedTokens() always returned 0.
function unlockVestedTokens() external returns (uint) {unlockVestedTokens(msg.sender);}function unlockVestedTokens(address _grantee) private returns (uint);
Update by the Colu team: Fixed before deploying to the [4725790] commit, added return to the overloaded method.
4. Issues of low severity
4.1 EllipseMarketMakerLib constructor (EllipseMarketMakerLib.sol)
The implementation of the constructor was using a low-level delegatecall to invoke a function on the library contract. We recommended replacing the usage of delegatecall for the Solidity function call. Manually crafting the function signature is highly error-prone, while the arguments are not type-checked. So, we suggested changing the approach of contract initialization as in the example below.
// EllipseMarketMaker.sol// an empty constructorfunction EllipseMarketMaker() public {}
// EllipseMarketMakerLib.sol// rename constructor() to init()function init(address _mmLib, address _token1, address _token2) public notConstructed returns (bool) {…}
// while MarketMaker instance creationlet mm = EllipseMarketMaker.new();await mm.init(lib, token1, token2);
The constructed field and the notConstructed modifier seemed redundant. To check it, use:
require(mmLib == address(0));We recommended removing redundant state variables and modifiers as it reduces gas costs — due to reduced storage — and greatly simplifies the code.
Update by the Colu team: The suggested change wasn’t introduced as the init\constructor method couldn’t use the same delegation mechanism as in the fallback method, because the lib address hadn’t been yet populated. The redundant constructed field was changed as suggested .
4.2 EllipseMarketMaker does not implement IEllipseMarketMaker
We recommended to reduce code duplication.
Update by the Colu team: The code duplication couldn’t be reduced due to the constraints imposed by the lib mechanism.
4.3 EllipseMarketMaker#fallback (EllipseMarketMaker.sol)
We suggested using the returndatasize and returndatacopy opcodes instead of hardcoding 0x20 size, as well as checking Dispatcher.sol. The current implementation might prevent handling results exceeding 0x20 bytes.
Update by the Colu team: returndatasize is a new change form the metropolis fork update, so the team will use it in the next EllipseMarketMaker release. For now, such an update is irrelevant as the return data size is no bigger than 256 bits.
4.4 IssueanceFactory#tokenPayable (IssueanceFactory.sol)
Removing the redundant tokenPayable modifier may help to reduce gas deploying costs.
function insertCLNtoMarketMaker(address) public tokenPayable returns (uint256)function extractCLNfromMarketMaker() public tokenPayable returns (uint256)
Update by the Colu team: The tokenPayable modifier was removed from the [4725790] commit.
4.5 ColuLocalCurrency constructor (ColuLocalCurrency.sol)
To comply with the ERC20 specification, a user should be allowed to create tokens with 0 decimals.
require(_decimals != 0);Update by the Colu team: The redundant require was removed from the [4725790] commit .
4.6 CurrencyFactory#mmLibAddress (CurrencyFactory.sol)
It can be a good idea to make mmLibAddress updatable and allow for creating a setter for this field. This can be useful if any changes in mmLib are needed in the future.
Update by the Colu team: In order to ensure the immutability of the Market Maker contract, this suggestion was not followed. Instead, the team decided to create a new one — if needed.
4.7 IssueanceFactory loops (IssueanceFactory.sol)
We recommended to ensure that for-loops were safe during the whole contract life cycle.
Update by the Colu team: for-loops were painted in the [4725790] commit.
Suggestions
On detecting the following code in tests, we highly recommended to avoid such comparisons:
assert.approximately(returnAmount.toNumber(),exchange.expectedReturn.toNumber(), maxErr.toNumber());One can use the BigNumber.js library for checking that the bit numbers are equal.
We also suggested reworking all the tests containing assert#approximately().
Audit summary
A few low-severity issues were found and explained. Corresponding changes were proposed to follow best practices and minimize potential attack surface.
Update by the Colu team: Most of the suggestions were taken into account and introduced in the [4725790] commit.
