-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512
- 1 - Table of Contents
- 2 - Introduction
- 2.1 - Authenticity
- 2.2 - Audit Goals and Focus
- 2.2.1 - Sound Architecture
- 2.2.2 - Smart Contract Best Practices
- 2.2.3 - Code Correctness
- 2.2.4 - Code Quality
- 2.2.5 - Security
- 2.2.6 - Testing and testability
- 2.3 - About the Daoact pre-ICO
- 2.4 - Terminology
- 2.4.1 - Likelihood
- 2.4.2 - Impact
- 2.4.3 - Severity
- 3 - Overview
- 3.1 - Source Code
- 3.2 - General Notes
- 3.3 - Contracts
- 4 - - Testing
- 5 - Audit findings
- 5.1 - Note Issues
- 5.1.1 - Event parameters not indexed
- 5.1.2 - Use of if(...) throw; instead of require/assert
- 5.1.3 - Inline precondition checks in place of modifiers
- 5.1.4 - Constructor does not check start/end preconditions
- 5.1.5 - Inconsistent division of responsibilities between internal and external funding methods
- 5.1.6 - Pointless fundingRaised / availableFunding methods
- 5.2 - Low Issues
- 5.3 - Medium Issues
- 5.3.1 - setEndsAt permits extension or restart of crowdsale
- 5.3.2 - Fallback function does not permit crowdsale contribution
- 5.3.3 - Finalize is subject to extortion attack by contract owner
- 5.3.4 - Unnecessary accumulation of funds in contract
- 5.3.5 - Incorrect use of
Transferevent in finalize - 5.3.6 - setFinalize permits changes to funding targets at any time before the crowdsale begins
- 5.4 - High Issues
- 5.5 - Critical Issues
- 5.1 - Note Issues
On 2017-07-15, Nick Johnson and Makoto Inoue performed an audit of the Daoact smart contracts. Our findings are detailed below.
I, Nick Johnson have no stake or vested interest in Daoact. This audit was performed under a contracted hourly rate with no other compensation.
I, Makoto Inoue have no stake or vested interest in Daoact. This audit was performed under a contracted hourly rate with no other compensation.
This document should have an attached cryptographic signature to ensure it has not been tampered with. The signature can be verified using the public key from Nick Johnson's keybase.io record.
This audit includes assessments of the overall architecture and design choices. Given the subjective nature of these assessments, it will be up to the Daoact development team to determine whether any changes should be made.
This audit will evaluate whether the codebase follows the current established best practices for smart contract development.
This audit will evaluate whether the code does what it is intended to do.
This audit will evaluate whether the code has been written in a way that ensures readability and maintainability.
This audit will look for any exploitable security vulnerabilities, or other potential threats to either the operators of Daoact or its users.
This audit will examine how easily tested the code is, and review how thoroughly tested the code is.
The Daoact pre-ICO is an uncapped crowdsale with no limits on contributor participation. There is no minimum fundraising goal below which contributions are refunded. Funds raised are sent 95% to one account and 5% to another after the crowdsale ends.
This audit uses the following terminology.
How likely a bug is to be encountered or exploited in the wild, as specified by the OWASP risk rating methodology.
The impact a bug would have if exploited, as specified by the OWASP risk rating methodology.
How serious the issue is, derived from Likelihood and Impact as specified by the OWASP risk rating methodology.
The Daoact smart contract source code was made available in the daoact/daoact-solidity Github repository.
The code was audited as of commit 40687a082781dd7adaaad0b9f65c2806c877b5cc.
The following Solidity source files (with SHA1 sums) were audited:
SHA1(ForecasterReward.sol)= 5268a25ac6fd3f1d0d24e60c898aabd6ab57db7f
Haltable.sol, Ownable.sol, and SafeMath.sol are common library code and were NOT audited. We did not audit any code for the accounts that will be recipient of crowdsale funds, or any code involved in subsequent allocation of tokens to users.
The contract is structured in a manner that takes in contributions from users during a fixed period, denoted by block numbers, and stores them until a finalize function is called after the crowdsale ends. When that function is called, it pays out 95% of the contributions to one predefined address and 5% to another. Each contribution is recorded by emitting events and recording the contribution on the blockchain in contract storage.
The contract imposes no restrictions on the code (if any) at those destination addresses, or on if and how tokens are subsequently allocated to contributors. As such, we did not audit either of these aspects.
ForecasterReward.sol is the main contract, and the only one reviewed as part of this audit.
No automated tests were provided by the authors. A document with manual testing procedures was provided. Makoto Inoue wrote basic unit tests in order to check the contract's behaviour in standard usage.
- Likelihood: low
- Impact: low
The Invested and Transfer contracts both contain address arguments that potentially benefit from indexing, but these arguments are not indexed.
- Likelihood: low
- Impact: low
Throughout the code, the if(...) throw; pattern is used. Solidity now permits require(...) (for checking data validity) and assert(...) (for checking other conditions), which facilitates the same functionality, but is significantly easier to read and audit.
- Likelihood: low
- Impact: low
Many preconditions are checked by contract code, particularly in the constructor and the finalize function, that would be more clearly expressed as contract modifiers.
- Likelihood: low
- Impact: low
The contract constructor checks that start and end block numbers are nonzero, and that the end time is after the start time, but does not prevent block numbers that are in the past. The code should be modified to prohibit deploying a contract whose start or end time has already passed.
- Likelihood: low
- Impact: low
Funding the contract is made possible via two external methods, invest and buy, both of which call an internal method, investInternal. invest contains its own precondition check for address, meaning that precondition checking is split between the internal and external methods. We recommend moving this precondition check to the internal method. This has the added benefit of removing the need for a separate external wrapper method, allowing investInternal to be renamed to buy and made public.
- Likelihood: low
- Impact: low
The fundingRaised and availableFunding methods act only as aliases for weiRaised and contract.balance, which are already publicly accessible.
- Likelihood: medium
- Impact: low
The contract uses block numbers to denote start and end times, based on projections of expected block time. This can lead to variance between the expected and actual start and end time of the crowdsale, a problem which is exacerbated by the difficulty of accurately predicting when a block will be mined given the oncoming ice age and variations in hashrate.
While others have at times recommended the avoidance of block timestamp as a timekeeping mechanism, the degree to which miners can influence timestamps is extremely limited; blocks must have a timestamp greater than the previous block, and blocks with timestamps in the future will be rejected by nodes and not propagated through the network. As a result, timestamps can be expected to be accurate to within a small degree of error (commonly several seconds, up to a few minutes in extreme cases) - and predicted block times rapidly become less accurate than the time given by even a malicious miner due to the expected variation in block intervals.
We recommend using timestamps instead of block numbers.
- Likelihood: low
- Impact: medium
Events emitted by the crowdsale contract provide all the data needed for the crowdsale owners to determine contributions by each user and later allocate tokens to them. However, the contract redundantly stores this information in list of ledger structs, a mapping keyed on contributor addresses, as well as storing global totals in state variables. Since this data is not required onchain, we recommend deleting the 'ledger', the mapping, and whatever global variables can be easily reconstructed from logs, to save gas and storage space.
We estimate eliminating these would save up to 115,000 gas per contribution transaction.
- Likelihood: medium
- Impact: low
The setEndsAt method enforces that the provided block number cannot be before the current block number. This makes terminating the crowdsale early problematic, as it requires the caller to predict when the transaction will be mined. We recommend removing this check, or modifying it to set the end time to the previous block number if that is larger than the supplied value.
- Likelihood: medium
- Impact: medium
The setEndsAt method permits the owner of the contract to extend the end time of the crowdsale arbitrarily. Because setEndsAt does not prohibit calls after the crowdsale has ended, the owner can even restart the crowdsale after it had previously completed.
We recommend prohibiting changing the end time after the crowdsale has ended, and suggest considering prohibiting extending the crowdsale in general.
- Likelihood: medium
- Impact: medium
Common crowdsale practice is to configure the fallback function to permit contribution to the crowdsale. This improves usability, as even wallets with full fledged support for smart contracts (eg, Mist) have significant usability barriers to calling contracts on a one-off basis, and most crowdsales have not developed web3 UIs specifically for contributing to the crowdsale.
We recommend modifying the fallback function to take the place of the buy function.
- Likelihood: low
- Impact: high
The contract owner's powers throughout the contract are generally quite limited: They can change the end time, disable new contributions in case of an emergency, and change the target addresses before the crowdsale begins.
However, because of the onlyOwner and stopInEmergency modifiers on the finalize function, the owner can prevent funds from being withdrawn from the contract to the target wallets entirely. This requires otherwise unnecessary trust in the good behaviour of the contract owner.
The finalize function can only be called once funding is over, is idempotent, and behaves in the same fashion regardless of caller. As a result, we recommend removing both modifiers, to ensure the funds can always be withdrawn to the authorised wallets after the crowdsale completes.
- Likelihood: low
- Impact: high
The contract stores funds received from contributors throughout the fundraising period, only disbursing them at the end when the finalize function is called. This increases the attractiveness of this contract to attackers, as it stores a significant amount of value.
In order to minimise this risk, we recommend refactoring the contract to send out contributions to the destination accounts immediately, in the buy or invest call.
- Likelihood: high
- Impact: low
On line 199, Transfer(forecasters, remaining) is called to log the transfer of funds. This should instead be Transfer(preICOContract, remaining).
- Likelihood: low
- Impact: high
The owner of the contract can call setFinalize at any time before the crowdsale begins, changing the target addresses for raised funds. This makes it impossible for a user to audit that the correct addresses are used in the deployed contract until the crowdsale has already begun.
We recommend removing this function entirely. In the event that changing these addresses is necessary, the contract should be redeployed with the correct address.
None found.
None found.
-----BEGIN PGP SIGNATURE----- Version: Keybase OpenPGP v2.0.73 Comment: https://keybase.io/crypto
wsFcBAABCgAGBQJZagg+AAoJEG2WSZMvKV0SgmAP/j13Phi/H9b4yb8PTLLkEZnX GoD0/GqUZvCnk7R3VVtAT/OfI459pa7aO3b4+NpA2/Zf3iyRaN4yFBq5v90Ak7On fBNYgXwEkctTXN4gGG5O+Bv1mT8ahxkCgjivr0ZvibU4TXxE7la5EqYywO6QJjLx piRb2C+/OmLOgsUDauOrP06vDcISRtymQ58l8kbdZiY4h8PRl0AeGpsHUskf3UFv 0r6C1Y39aylGrH3eA6gM7/fkZTFcL+3ffVsB1JOnuczilBqebrSgKnEu1MQEtHLE Vcdy3Ry3nmVkeL5K2nxkpNCKjHSMfO6jpyMbalPRSwFNEA2FDJXDgSXMo/nGlTbG /lVBJccjlS//BUV2BpcwnEnDtiHjKnFCWqd0FuyMDruCeylEyqE6J13RXjfd+kMt eEXSG05NH8t/qZccPZr4oFX/5eq8tJzEFyqbR5qguiuOD1/7OGEwsWLIzTWPRDd2 JwL660V8WQ5Afw+stN50ul+hB8IEPkB9V8I+T1KdfJbhSdYntOdA3BmsXd4sG8uw QlAtWocBoOCP7PYk+GpqJwQvWDOA1uL7b366QMlrydQvUib3LuV2Eay3zILWMye9 t9MHV4a1RGswQoiJ1SXq7ou8zfFZLkVAMtR3RIWN5KuWQ2/IW+KpWqcwttbBVZe+ 1IVWwXrgKm1M/YfYdX3n =O9Az -----END PGP SIGNATURE-----