Matic Network Security Audit Report

Nomic Labs audit report on Matic Network’s token and vesting smart contracts

Summary

We conducted a security audit of Matic Network’s ERC20 token and token vesting smart contracts. The audited version is 5d5f39516b89d96bca941a54e475e120f2d21f18.

We found two medium security issues and one low severity issue in the TokenVesting contract. Additional comments and recommendations were made around the implementation. All issues have been properly addressed by the Matic team.

Audit Results

Introduction

During this audit, we assumed that TokenVesting#_token will contain the address of an instance of Token, and that its owner role will be in control of Matic Network.

Medium severity issues

[MAT01-M01] Receiving tokens in excess is not handled by the TokenVesting contract

If more tokens than expected are sent to TokenVesting, there’s no clear way to retrieve them. A vesting entry can be added in order to get the tokens back, but this can create confusion with the different stakeholders of the system.

We recommend having an explicit method to retrieve exceeding tokens. Implementing this would require keeping track of how many tokens are supposed to be in control of TokenVesting at any given time, but this should be fairly straightforward.

Update: The function retrieveExcessTokens was added to handle this case.

[MAT01-M02] Canceling a vesting entry doesn’t handle its tokens

The function TokenVesting#removeVesting makes a vesting entry non-releasable, leaving its tokens in the TokenVesting contract. This creates a situation very similar to [MAT01-M01].

We recommend sending the tokens associated with the vesting being canceled to an address in control of Matic, possibly the owner of TokenVesting.

Update: The new retrieveExcessTokens can be used to recover tokens from associated with a canceled vesting entry.

Low severity issues

[MAT01-L01] Receiving fewer tokens is not handled by TokenVesting

The TokenVesting contract distributes tokens via its release function. This function sends the entire amount of tokens of the vesting entry at once, failing if there aren’t enough.

If the contract accidentally receives fewer tokens than expected, the last beneficiary of the contract won’t be able to retrieve their tokens. This would probably happen years after the contract is deployed, which would create further confusion.

While confusing, this situation can be fixed by Matic by sending more tokens to the contract. And it can be prevented with enough testing.

Another alternative worth considering is releasing as many tokens as possible and keeping track of how many were missing. This way the last beneficiary would be able to get some of their tokens sooner while waiting for Matic to give them the rest.

Update: The TokenVesting contract has been updated with real data, that reflects the contract getting the entire supply of tokens over the full release period, which fully mitigates this scenario.

Other comments and recommendations

[MAT01-O01] Contract names should be more descriptive

We recommend using more descriptive contract names, as they will be shown out of context in places like Etherscan. For example, they could be named MaticToken and MaticTokenVesting.

Update: The contracts have been renamed as suggested.

[MAT01-O02] Solidity version pinning

We recommend pinning the version of Solidity in the contract code. This was already done in the Truffle config, but doing so in the code will make it more explicit.

Update: The contracts now specify Solidity 0.5.2.

[MAT01-O03] Update evmVersion in solc’s config

The contracts are compiled targeting the Byzantium EVM. We recommend upgrading to Petersburg.

[MAT01-O04] TokenVesting#removeVesting doesn’t emit any events

We recommend emitting an event when removing a vesting entry. Vesting beneficiaries would probably want to monitor this operation, and an event would make it easier.

Update: An event was added to this function.

[MAT01-O05] TokenVesting#vestingId could be private

We found no reason for this field to be public, and we recommend making it private.

Update: This field is now private.

[MAT01-O06] Most require calls have no error message

We strongly recommend adding clear error messages to every require call. This not only helps with debugging, but it is also helpful for users interacting with the contracts directly.

Update: Every require() has a clear error message now.

[MAT01-O07] Shared state between tests

The current tests of TokenVesting depend on each other, this is a bad testing practice, as it can easily hide bugs.

We recommend not using Truffle migrations for testing, but deploying new instances of the contracts in each test.

Update: Tests don’t share state anymore.

[MAT01-O08] Untested functionality

We recommend increasing the test coverage of the TokenVesting contract. These are some cases that we consider interesting to test:

1. Removing a vesting entry with the owner account.
2. Removing a vesting entry with a non-owner account.
3. Trying to remove an unexistent vesting entry.
4. Trying to remove an already released vesting entry.
5. Trying to remove an already removed vesting entry.
6. Trying to add a vesting entry from a non-owner account.
7. Trying to release the tokens associated with an already released vesting entry.
8. Trying to release the tokens associated with a canceled vesting entry.
9. Trying to release the tokens associated with an unexisting vesting entry.
10. Trying to release the tokens associated with a vesting entry when there aren’t enough tokens in the TokenVesting contract.
11. Trying to release the tokens associated with a vesting entry when the balance of TokenVesting is exactly the same than the entry’s amount.
12. Sending more tokens to TokenVesting than the total amount of those associated with the vesting entries.
13. Creating a vesting entry without sending its tokens to TokenVesting.

Update: The vesting contract tests have been rewritten. They now cover cases: 1, 2, 3, 4, 5, 6, 7, 10, and 11. The test “Trying to release the tokens associated with existing vesting entry” is incorrectly named, but functional.

[MAT01-O09] Failing TokenVesting tests

There are three failing tests of TokenVesting. We believe this is because testing data was added in TokenVesting’s constructor after implementing those tests, without updating them accordingly.

Update: All tests are passing on commit 274c847a09.

Deployment recommendations

We recommend creating a deployment script with the real values to be used (except maybe for addresses if they should be private for now) and create automated tests for it.

Things that should be tested are:

1. Each entry’s values.
2. TokenVesting’s ownership.
3. The total amount of expected tokens by TokenVesting and its actual balance.

We found the pattern of including the calls to addVesting in the TokenVesting’s constructor to be helpful. Keeping it for the actual data would simplify the deployment script considerably, as it would only need to send transactions for:

1. Deploying the Token contract.
2. Deploying the TokenVesting contract.
3. Transferring tokens to the TokenVesting contract.
4. Transferring TokenVesting contract’s ownership to a Matic-controlled address.