Coinvest Token V2 Audit

Principal Auditor:
Alexander Wade
Published:
May 4th, 2018
Download Report

1. Overview

This document serves as the official audit report of a token contract, created and published by Coinvest. Coinvest is attempting to offer a forked version of their COIN token, in order to migrate their COIN token from ERC223 compliance to ERC20 and ERC865 compliance.

The project audited is Coinvest’s ERC865 implementation, which attempts to further the work done towards finalizing the ERC865 standard, and stretch it to its logical conclusions. The mechanism in play is a token swap, wherein users transfer their COIN V1 tokens to a contract address, which automatically transfers them an equivalent amount of COIN V2 tokens.

The ERC865 implementation used by Coinvest seeks to allow their token holders to transfer their COIN tokens without needing any ETH. ERC865, although not finalized, uses unique data signature structures to allow users to delegate on-chain transfers of their COIN V2 tokens to a third party. This execution is paid for using an agreed-upon rate in COIN V2 tokens.

This audit considered not only the security and efficiency of the existing contracts, but also the mechanism of the V1 to V2 token swap, and the potential changes to ERC865 that could leave COIN incompatible with other 865 tokens down the line.

2. Introduction

2.1 Authenticity

The audited contracts are in the Coinvest COIN repository, in the Token folder: https://github.com/CoinvestHQ/COIN/tree/master/Contracts/Token.

The initial version used for this audit is commit: 56694cca885963be568104c0c5b35505be1c07e4.

Over the course of the audit, and within scope of the audit, the Coinvest team released more commits, the most recent of which is commit: 3dfa6216b85e2b16a5cd1ac770deb4f3e7e6e05e.

The main contract for the COIN V2 token is located in CoinvestTokenV2.sol. The contract used for the V1 to V2 swap is located in TokenSwap.sol.

2.2 Scope

This audit covered only the files CoinvestTokenV2.sol and TokenSwap.sol. Although other contracts were read through, they are considered out-of-scope for the purposes of this audit.

2.3 Methodology

This audit focuses heavily on not only inspecting the provided smart contracts for vulnerabilities and potential for losses in funds, but also on working closely with the Coinvest team to ensure their COIN Token V2 contract is as forward-compatible with the to-be-finalized ERC865 standard as possible. The end goal of this audit is to help the team not only secure their contracts, but also to ensure their vision for the project is best represented by the project they put forward. As a result, additional concerns such as efficiency and design are included in the notes section of this report as well.

2.4 Terminology

OWASP Chart

This audit categorizes vulnerabilities using the OWASP risk rating method based on impact and likelihood. Each vulnerability is given impact and likelihood scores, which are used to give a more accurate estimation of a vulnerability’s overall severity. An additional factor in severity is the relative ease with which a vulnerability is fixed: an issue which requires extreme refactoring will be weighted higher than one with the same severity which is a quick fix.

2.5 Disclaimer

This document reflects the understanding of security flaws and vulnerabilities as they are known to the Authio team, and as they relate to the reviewed project. This document makes no statements on the viability of the project, or the safety of its contracts. This audit is not intended to represent investment advice.

3. Findings

3.1 General

Overall, the implementation presented by the Coinvest team was clean and well-planned. The majority of issues found revolved around the ERC865 implementation. Ensuring compatibility going forward of an unfinalized ERC standard is no small feat. During the review of the existing standard, we found the ERC865 interface to be lacking in some respects, and flawed in others. Suggestions made during the course of this audit attempted to best correct for these issues while not undermining the future compatibility of COIN by introducing large differences between the COIN ERC865 implementation and the implementation currently described in the standard.

3.2 Contract Explanation

3.2.1 Function - Token

CoinvestTokenV2.sol: This contract houses the logic implementing the COIN V2 Token. COIN V2 is ERC20 compatible, and implements the current interface for ERC865 as well. Users interact with this contract the way they would any token contract, with the additional functionality of the pre-signed off-chain delegated execution system.

3.2.2 Function - Token Swap

TokenSwap.sol: This contract houses the logic required for users to easily exchange their COIN V1 tokens for V2 tokens. Users simply transfer their COIN V1 tokens to the TokenSwap contract, during which the tokenFallback function will be called. The token fallback function ensures that the sender is the V1 contract, and transfers V2 tokens to the sender equal to the number of V1 tokens sent.

3.3 Critical Priority

No critical priority issues were found.

3.4 High Priority

No high priority issues were found.

3.5 Medium Priority

Delegate execution fee overflow: In each of the PreSigned functions, fee paid is calculated as the amount of gas burned during the delegated operation plus 35,000, multiplied by the signer’s set gas price in tokens. This multiplication does not use SafeMath, and so is vulnerable to manipulation by the signer, who can make it appear as if they are offering a substantial fee for transaction execution while spending far less due to the overflow.

We recommended simply using SafeMath in place of a standard multiplication operator. The Coinvest team agreed, and the issue was resolved in commit 530d03b.

Delegated approval racing condition: The standard ERC20 implementation contains a widely-known racing condition in its approve function, wherein a spender is able to witness the token owner broadcast a transaction altering their approval, and quickly sign and broadcast a transaction using transferFrom to move the current approved amount from the owner’s balance to the spender. If the spender’s transaction is validated before the owner’s, the spender is able to spend their entire approval amount twice.

Allowing a third party to handle token approvals exacerbates this problem substantially, as the delegate who receives the signed approval from the delegating party is able to create these racing conditions with a much higher success rate then before. As approvePreSigned and approveAndCallPreSigned are both part of the current ERC865 interface, the best solution seemed to be to implement additional functions which did not have this vulnerability. The functions to be implemented are increaseApprovalPreSigned and decreaseApprovalPreSigned. These two functions all rely on the current allowance value of the spender – and ensure the token owner does not unintentionally create a scenario in which the spender is allowed to perform a double-spend from the owner's balance.

Additionally, we recommended implementing a function revokeSignature, which would allow a signer to revoke the validity of an existing signature. This function, as well as the previous functions, would all utilize an event SignatureRedeemed. These suggestions were all compiled and will be posted as a comment in the ERC865 GitHub post for open discussion.

The Coinvest team implemented these suggestions to our satisfaction over the course of several commits.

3.6 Low Priority

approve unnecessary bounds check: The approve function checks that the owner approving an amount for the spender has a balance at least equal to the amount being approved. As this is not explicitly required by the ERC20 standard and can easily be circumvented by approving and then transferring tokens, the check does not serve a purpose and should be removed. This issue was fixed in commit fc43897.

ERC865 signature hash function selector match: Among other parameters, a valid delegate-able transaction should sign the function selector of the intended function to be called by the delegate. The function selectors in the linked code reference ERC865 functions, but the current ERC865 implementation uses the function’s corresponding ERC20 counterpart functions instead.

We recommended switching the pre-signed hash values to use the same function selectors as the ERC865 implementation. The Coinvest team agreed, and the issue was resolved in commit  530d03b.

Potential manipulation of COIN V2 token supply and balances: The current COIN V1 to V2 swap process involves some address deploying the COIN V2 contract; this address is awarded the entire supply of the COIN V2 token. Next, some address must deploy the swap contract and set appropriate addresses for the old and new token contracts. The first address must then transfer the COIN V2 tokens to the swap contract. Finally, COIN V1 token holders are able to transfer their tokens to the swap contract, which automatically transfers the corresponding amount of V2 tokens in return. The successful migration of every token holder’s balance depends on the deployer of the COIN V2 contract transferring the entire total supply of the new V2 token to the swap contract. Otherwise, the swap contract will be unable to fulfill each user’s request for a swap.

While it is unlikely such a scenario would go unnoticed, it is best to address this within the code, instead of outside of the code. We recommend having the COIN V2 contract be deployed by the swap contract, which will automatically award the swap contract with the entire V2 total supply – without needing to rely on some third party. This issue was fixed in commit 4c302c5.

3.7 Notes and Recommendations

Mark variables constant, where applicable: Both TokenSwap.oldToken and CoinvestTokenV2._totalSupply should be marked constant. Reading from constant variables is much cheaper than reading from a state variable – and additionally ensures that these values will never change.

Fix compiler warning – unused variable: TokenSwap.tokenFallback currently produces a compiler warning due to the unused _data parameter. This warning should be silenced by adding the line _data; to the function in question.

Marked version pragma incompatible with code: The COIN V2 contract signifies a compiler version of 0.4.20 or greater. However, compiling with any version less than 0.4.23 will result in warnings. Consider changing the version pragma to use 0.4.23 and above.

Update Ownable contract to latest Constructor syntax: CoinvestToken was updated to use the new constructor keyword. In order to maintain the same standard throughout the code, update Ownable to use the new syntax as well.

Explicitly state visibility modifiers for state variables: CoinvestToken has several variables that should be explicitly marked as internal, for clarity.

Transfer function should check that the recipient is nonzero and is not the COIN V2 address: Given the potential for confusion by users during the token migration, adding these additional checks will help ensure tokens reach their intended destination.

4. Documents & Resources

4.1 Line-By-Line Comments

4.2 Project Codebase

4.3 External References

5. Conclusion

The Authio team would like to commend Coinvest on their COIN V2 token and swap contracts. Aside from minor exceptions, contracts were updated to the most recent version of Solidity, silencing the majority of compiler warnings. This quick update to the latest released version shows a commitment to staying up-to-date, and bodes well for the security and auditability of smart contracts down the line.

We would like to recommend that Coinvest continue with the process of securing their code by posting public bug bounties and soliciting community feedback. More eyes are always better – especially in the case that the standard COIN is built on is not yet finalized.