Aragon Staking Security Audit

Executive Summary

The assessment was conducted on the Staking and StakingFactory contracts from the Git repository at https://github.com/aragon/staking as of commit c7537c4519930ca254f693ff4e65117619b08f23 tag audit of September 10th.

The code was found to be clear, well written, well commented, and very well tested.

The following issues were identified during the assessment:

The Staking and StakingFactory contracts were found to work as expected, and no major issues have been found.

Introduction

The scope of the audit was limited to the following Solidity source files, shown here with their sha256sum hash:

Assesment

contracts/Staking.sol

43:8 warning Line exceeds the limit of 145 characters max-len

✖ 1 warning found.

The contracts are compiled with Solidity version 0.5.17, which is the latest maintenance release of the 0.5.x series.

The repository includes 149 tests, and all tests pass. According to solidity-coverage the tests have full coverage of the contracts in the assessment’s scope. However, it was found that the tests don’t verify all emitted events (see ARS-001).

The Staking contract implements the staking standard interfaces IERC900 and IERC900History, as well as ILockable, IApproveAndCallFallBack. It also inherits from the utility contracts IsContract and TimeHelpers. It uses the Checkpointing, SafeERC20 and SafeMath libraries.

The StakingFactory contract creates and keeps track of a unique Staking contract for each ERC20 token contract.

A Staking contract is always associated with a fixed ERC20 token, and it allows any user to stake an amount of tokens, effectively transferring them to the Staking contract. Then the user can give an allowance to a lock manager contract, which then has total control to lock or unlock up to the allowed amount of tokens. Note that there’s no mechanism to recover locked funds, and it’s completely up to the implementation of the lock manager contract.

It is important to mention that the implementation of a lock manager is very relevant to security. In particular, the Agreement contract is a lock manager shared between all the disputable apps linked with it. Since the lock manager is shared between the disputable apps, depending on the externally accessible functions of the lock manager a disputable app could maliciously lock or unlock funds of a user of another disputable app. This is why the Agreement contract assumes that any disputable app linked to it is honest (as to not to lock funds maliciously). In general, it is advisable to avoid sharing lock managers.

The Staking and StakingFactory contracts were found to work as expected, and no major issues have been found.

Summary of Findings

Findings

Description

The following events emitted by the Staking contract are never tested: Staked, Unstaked, StakeTransferred, NewLockManager, LockAmountChanged, LockAllowanceChanged, LockManagerRemoved, LockManagerTransferred.

Recommendation

Although test coverage is already very good, it is recommended to improve coverage by including tests for the missing events. The tests should verify that the events are emitted when expected and the event parameters are correct.

Disclaimer

Security for a Decentralized World