Aragon Staking Security Audit
Executive Summary
In August 2020, Aragon engaged Coinspect to perform a source code review of the new Staking app 0.3.0. The objective of the audit was to evaluate the security of the smart contracts.
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 audit started on September 14th and 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 scope of the audit was limited to the following Solidity source files, shown here with their sha256sum hash:
Assesment
The contracts compile without problems, and linting shows one minor warning:
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
The present security audit does not cover the endpoint systems and wallets that communicate with the contracts, nor the general operational security of the company whose contracts have been audited. This document should not be read as investment advice or an offering of tokens.
