The Brave team asked us to review and audit their new BAT Token contract code. We looked at their contracts and now publish our results.
The audited contracts can be found in their basic-attention-token-crowdsale repo. The version used for this report is commit 17a5f8440a256a6dc5d8dd894b9615182c2901b2.
Here’s our assessment and recommendations, in order of importance.
Update: Brave team followed most of our recommendations in the latest version of their code.
A fixed 300 million BAT tokens are assigned to Brave (specifically to the batFundDeposit address) when the crowdsale contract is deployed. If the contract doesn’t reach the minimum amount of tokens (tokenCreationMin), though, it goes into a state where investors can request a refund and receive their ether back.
In this situation, Brave would also be able to request a refund for the 300 million tokens it holds, taking ether that should be available for investors.
Fix this by checking that funds aren’t sent to batFundDeposit in the refundfunction.
Update: fixed in commit 09a1038c2ac0148b93876057fc056ae2a872fb88.
Brave has decided to use block numbers instead of timestamps to determine the start and end of the crowdsale, which is a good security practice. However, preconditions about them are missing in the contract constructor.
For example, consider checking fundingStartBlock comes before fundingEndBlock, and that both are in the future (greater than block.number).
The crowdsale contract uses the Transfer event to log the creation of tokens, with a from field set to the zero address. This is overloading the semantics of the Transfer event of the ERC20 standard, which might not be a good idea as other software (such as wallets or exchanges) could be relying on these semantics. This is an ongoing discussion in the ERC20 standardization process, but our stance on it is that a different event should be used.
Update: fixed in commit 09a1038c2ac0148b93876057fc056ae2a872fb88.
The contracts Token, StandardToken, and SafeMath are very similar to OpenZeppelin library contracts ERC20Token, StandardToken, and SafeMath, respectively. Consider avoiding code repetition, which can bring regression problems and introduce unexpected bugs. Consider using the standard modules from OpenZeppelin.
Several conditional checks, such as block.number < fundingEndBlock, are repeated throughout the code (line 51, line 68, line 79). Consider extracting these to reusable internal functions with declarative names (for example isWithinFundingPeriod) for better code readability.
Most math operations are safe, but there’s still a few that are unchecked (these two, for instance). It’s always better to be safe and perform checked operations.
Update: fixed in commit 09a1038c2ac0148b93876057fc056ae2a872fb88.
The amount of BAT assigned to Brave is fixed at deploy time. This seems to be inconsistent with the process document we received that states: “In event of a non-sell out, contract will ‘burn’ BAT kept in multisig wallet down to a 3/10 ratio of total BAT.”. Consider implementing this in the contract, as the specification states, so that the Brave team doesn’t have to do this manually after the token sale.
A few constants in the contract which denote BAT amounts (batFund, tokenCreationCap, tokenCreationMin) are written assuming 18 decimals. This is a hidden case of a magic constant. These amounts should be parameterized by the number of decimals (e.g. batFund = 300 * 10**6 * 10**decimals).
Update: fixed in commit 09a1038c2ac0148b93876057fc056ae2a872fb88.
The condition in line 68, is expressed in a very confusing way. It should be split into two conditional expressions: totalSupply < tokenCreationMin and block.number <= fundingEndBlock && totalSupply != tokenCreationCap, each of which throws when true.
Update: fixed in commit 09a1038c2ac0148b93876057fc056ae2a872fb88.
The variable isFunding declared in line 18 doesn’t really reflect whether the contract is funding or not. This fact depends on other conditions, such as being within the funding period. The variable marks the finalization by the contract owners, so a good name would be isFinalized (note that this new name reverses the meaning: isFinalized == !isFunding).
There’s a comment in line 18 that says the variable is “no longer important”, but it is actually a key part of the crowdsale process. Consider removing that comment.
Update: fixed in commit 09a1038c2ac0148b93876057fc056ae2a872fb88.
There’s two uses of var, where the type of the variable is inferred from the expression on the right hand of the definition. We recommend avoiding this feature because in some cases it might infer a smaller integer type than the developer might think. It is best to be explicit regarding types.
Update: fixed in commit 09a1038c2ac0148b93876057fc056ae2a872fb88.
All of the contracts require version 0.4.10 of Solidity. It should also be noted that 0.4.11 was released a few days ago. Consider changing the solidity version pragma to the latest version (pragma solidity ^0.4.11;) to enforce latest compiler version to be used.
As an additional note, since the contracts are compiled with a recent version of Solidity, it’s possible to use the new transfer method instead of send(here, and here).
finalize function no longer issues BAT tokens to the User Growth Fund. These tokens seem to be issued in the constructor.batFund in line 21 is only set at deploy time, and never changed, so it could be declared constant.finalize function can be called from any account, but this is neither what the code does, nor what the review document says.tmpSupply variable instead of recalculating the sum on line 56.One severe security issue was found and explained, along with recommendations on how to fix it. Some additional changes were proposed to follow best practices and reduce potential attack surface.
Update: Brave team followed most of our recommendations in the latest version of their code.
Thanks to Francisco Giordano for helping write this report.