Aragon 0.5 Audit by Adam Dossa
This was the initial review of the aragonOS system that powers the v0.5 release of Aragon. Further audits and public bug bounty programs are scheduled to begin in early January 2018.
The original PDF report has been converted to Markdown for easier reading. You can download unmodified report from the auditor here:
aragon-core has been renamed to
aragonOS after this audit took place and that change has been reflected in this document. See the PDF file above for the untouched original report.
This audit report was undertaken by @adamdossa for the purpose of providing feedback to Aragon.
It has been written without any express or implied warranty.
The review focussed on the Solidity contract code in these repos, and covered:
- possible attack vectors
- mismatches between logic and documentation
- possible code improvements
- confusing functionality or naming
The review was performed using the code tagged with
audit1 at the following commits.
We used RocketChat to coordinate the review and help resolve questions during the review process.
Overall the code structure was readable, well-documented and structured coherently and consistently.
Functions largely conform to the best practices at:
making the structure and code easy to parse and understand.
The codebase also included comprehensive testing suites, using the Truffle testing framework. The Aragon team also employs a coverage framework and linter process to ensure their code remains consistent and testing comprehensive.
The team was responsive, measured and prompt in their responses and feedback during the process.
- There were a few functions that were missing explicit function visibility modifiers. This has been
reviewed and corrected (including linter updates) via:
- In addition to the below there are a few miscellaneous fixes and improvements that have been made, including additional testing coverage and minor fixes. These include:
as well as many other incremental improvements which have been made during and after this audit process.
These contracts form the core Kernel implementation. The Kernel manages app and Kernel permissions and provides a layer of indirection to access apps (allowing apps, including the Kernel, to be upgraded as necessary).
There were a few areas of the documentation which were either out-dated, or possibly confusing.
- description of
Grant Permissionwhich has now been amended to make the conditions of this operation clearer.
- making the separation between instantiation and initialisation clearer.
- correcting some minor mistakes in the example code for
Some possible future improvements were identified, specifically:
- events being triggered when permissions were used.
- managing the instantiation / initialisation of contracts (see below section for more details).
- continuing to work on documentation to make the permission framework as clear as possible.
aragonOS/contracts/apps and aragonOS/contracts/common
These contracts contains the logic required for both proxy contracts (which allow the underlying "business logic" contracts to be upgraded) and forwarding (which allows entities to forward transaction scripts to other entities for execution).
- Pre-Byzantium logic could now be removed from
returndatasizeopcodes are now available. This has now been resolved via:
- The use of
delegatecallto execute underlying App logic is a clever way of preserving state and Ether value in the
DelegateProxywhilst using the code / business logic of an upgradable
Appcontract. The below represent areas that could be clarified in the documentation, rather than bugs in the code.
- Since the
Appcode is expected to have state / storage, this imposes a limitation that any new upgraded version of the
Apphas a superset of the previous contract which it is replacing.
- Since state is maintained in the
DelegateProxycontract, not the underlying
Appcontract, this may cause some practical issues with third-party apps such as exchanges or etherscan.io (for example I don't believe the "Read Contract" functionality on etherscan.io would work as the ABI wouldn't have the relevant functions on the
DelegateProxycontract, and the state on the
Appcontract wouldn't reflect updates).
- There were a few areas of documentation which use inconsistent terminology (e.g. scaling vs. forwarding) which have been resolved via:
- One of the identified issues related to the instantiation and initialisation of
AppProxycontracts not being atomic. In the expected use-cases, where
AppProxycontracts are instantiated and initialised through a factory process (within a single atomic transaction) this is not problematic, but there is some potential for this structure to be abused in unexpected use-cases, allowing a malicious user to front-run
AppProxycontracts. The Aragon team discussed various options internally and an approach was implemented to allow an optional
initializePayloadto be passed to the
AppProxyconstructor, which can be used to initialise the contract as part of its instantiation.
- Given the use of
delegatecall, the documentation could clarify the intention as to whether
Appcontracts are designed to be used as factory contracts, or redeployed for each Aragon organisation.
- It may be sensible to make the
AppProxya variable that can be modified to account for any future changes to opcode gas costs for
This contract allows votes to take place on arbitrary actions, using the MiniMeToken to determine voting power of token holders.
Absentdoesn't seem to be used anywhere - not sure if this is in for some future purpose perhaps, if not it could be removed.
Votingapp automatically executed a vote payload once the outcome of the vote was beyond doubt. Whilst this is useful functionality, it made it hard for a voter to estimate gas requirements and the effect of their vote transaction deterministically (as the outcome depended on whether or not the vote tipped the balance). A boolean parameter has now been introduced to allow a voter to specify whether or not they want their vote to automatically execute the vote payload (if appropriate) via:
- Currently the duration of a vote is fixed on initialisation of the
Votingcontract - it may be worth considering making this a per-Vote variable instead to allow for both long and short-dated votes. Since the
_minAcceptQuorumPctcan be modified during the lifetime of the contract, it may be that users also want to change
- The event
CastVoteshould include the number of votes being cast to allow easier off-chain tracking of vote status. This has now been resolved via:
- When modifying the
changeMinAcceptQuorumPctit may be worth emitting an event for this change. This has now been resolved via:
- When creating a new vote through the
forwardmethod, it may be worth putting a non-blank
_metadatain to make it clearer to track where the vote has come from. This could incorporate the forwarding entities address.
This contract manages token actions through the MiniMeToken contract (either wrapping another token, or natively).
canForwardallows holders of tokens which have not yet vested to forward request - this may be intentional, but worth clarifying in the documentation.
- It is technically possible for a single
TokenManagerto be set as the controller for multiple
MiniMeTokencontracts. It may be worth asserting in the
msg.sendermatches the expected
tokenvalue, to avoid any misconfiguration.
- The vesting model follows broadly the Zeppelin approach. In my experience clients have often wanted a simpler model with a single cliff date, upon which all tokens vest. This is not possible to achieve with the implemented approach as the tokens must vest linearly between the start and vesting dates (with nothing vesting before the cliff date).
- It may be worth mentioning in the documentation that it is important to set the
MiniMeTokento be the
TokenManager(i.e. that it isn't sufficient to initialise the
MiniMeTokenbut that the link needs to go both ways).
This contract acts as a simple way to permission a group of entities as a single entity.
- In the documentation, there is a minor grammatical mistake - the phrase "This is needed for identification purposes, given that the only other context the group possesses." should perhaps be "This is needed for identification purposes, given that it is the only context the group possesses.".
This contract is responsible for raising funds (in any ERC20 token, which could include wrapped Ether) in return for a new ERC20 token.
- In line 297, it should be
(period.finalPrice != initialPrice)rather than
(period.finalPrice != 0). From the documentation, it says:
"A period has an initial and final price. If they are not the same, price for a given timestamp is linearly interpolated in function of time." This has now been resolved via:
- In function
_buythe calculation of
returnAmountis subject to rounding errors I believe. i.e. in the
isInversePricecase, if dividing by
pricePrecisionleads to rounding, then re-multiplying by
pricePrecisionmay not yield the expected value. This is an issue that is worth being aware of, but is unlikely to lead to any genuine problems.
initializefunction could check that the
TokenManageris running in native (rather than wrapped) mode by checking the public variable
_tokenManager.wrappedToken == 0.
getCurrentPricebehave differently on a
sendTransactionmay be confusing. You could refactor this so that it is genuinely a
constantfunction instead. I agree that functionally this works as
transitionSalePeriodIfNeededis always called before any other state modifying functions execute.
calculatePricehas an implicit dependency on
transitionSalePeriodIfNeededbeing called before it is used. This could be enforced in the function (e.g. by checking that the current timestamp is between
sale[currentPeriod].periodEnds. This is redundant in the current code (since
transitionSalePeriodIfNeededis always called before
calculatePricebut would make it harder to make errors in the future, or in derived contracts). This has now been resolved via:
- Adding a token purchase function which allows a beneficiary address (rather than
msg.sender) to receive tokens may be useful.
- Having the ability to associate a script to be called on a sale closing may be useful. This script could for example ensure that no more tokens could be minted (effectively capping the issued tokens supply) by revoking
MINT_ROLEpermissions from the
TokenManager, or allow the
FundRaisingcontract to transfer
CREATE_SALES_ROLEpermission to a different entity (i.e. a
VotingAppassociated with the token being issued).
- Maybe worth making the distinction between tokens being raised, and tokens being issued clearer through consistent variable naming. Tracking which variables correspond to
raisedTokenand which correspond to tokens being issued was confusing (maybe consistently call this
issuedTokenor similar). i.e.
uint256 returnTokens = ...could become
uint256 returnRaisedTokens = ...,
sale.minRaisedand so on.
- I'm not sure what the motivation to have multiple (potentially concurrent) ongoing sales is? I guess if you wanted to raise from a mix of tokens that would make sense (as each
Salecould have a different
raisedToken), but in this case you wouldn't have a consolidated cap etc., which would make it confusing. I can see that it may be required to have a follow-on sale, but wonder whether having each sale being represented by a new FundRaisingProxy may be simpler / clearer.
This contract is responsible for tracking transactions (payments or deposits) across an organisations token balances, and managing scheduled payments.
Given the importance of this contract and the value there would be in exploiting it, it may be worth having this specific contract be audited separately by multiple individuals and / or placing a bounty within a live deployment to encourage investigation.
- Any reason why the minimum payment duration is 2, rather than 1:
require(_periodDuration > 1);I can see that you use
settings.periodDuration - 1as the duration length, but not clear why you don't just use
settings.periodDuration(with the requirement that
settings.periodDuration > 0);
- In both
token.transfermethods could potentially re-enter the
Financeapp. I can't immediately see how this can cause an issue, but worth being aware of.
- It may be worth mentioning in the documentation that payments can be "back-dated", in which case the receiver will be eligible to execute all "back-dated" payments. This has now been resolved via:
- If the budget for a token is reduced during a period (via
setBudget) it is possible that
_getRemainingBudgetmay throw if the amount already spent (
periods[currentPeriodId()].tokenStatement[_token].expenses) is larger than the new budget (in which case
settings.budgets[_token].sub(spent)will throw). It may be better to return 0 rather than throwing in this case.
_makePaymentTransactioncould record the payment reference in the corresponding transaction reference.
- There are no restrictions on what ERC20 tokens can be used for deposits. This would allow an attacker to create an ERC20 token which always returns
_token.transferFromwithout actually depositing any tokens. Whilst this isn't really an issue it would be possible to bloat the
transactionsarray. Not sure there is really an easy solution to this, although having an optional "whitelist" of allowed token addresses that can be deposited is an option.
This contract is used to store an organisations tokens (and wrapped Ether). Any actual balances will be held in the corresponding
This is a nice and simple contract, with no obvious issues.
- Potentially integrating this contract with the
Financecontracts budget functionality. In other words setting a budget in the
Financeapp could require a corresponding allowance in the
Vaultapp for the given token.