The Simple Token team asked us to review and audit their Simple Token Sale contracts. We looked at the code and now publish our results.
The audited code is located in theOpenSTFoundation/SimpleTokenSalerepository. The version used for this report is commit
ProcessableAllocationscontract is an internal tool and outside of the scope of this audit.
Here’s our assessment and recommendations, in order of importance.
Update: The Simple Token team has followed most of our recommendations and updated the contracts. The new version is at commit
No issues of critical severity.
No issues of high severity.
Sale can end while being paused
hasSaleEnded function can change its return value from false to true during the pause period while there should be an extension of the duration of the token sale. The update of
endTime is done only after calling the
unpausefunction and the state during the pause period may be misreported.
A consequence of this could be losing the ability to update whitelist in
updateWhitelist function during the pause period.
We recommend updating
hasSaleEnded function to consider the pause period.
Update: Fixed in these two commits.
Unchecked assumption that trustee and token have the same owner
reclaimTokens function, there is a token transfer to the
trusteecontract owner, but according to the comment above it should be transferred to the
tokenContract owner: Note that the trustee should be able to move tokens even before the token is finalised because SimpleToken allows sending back to owner specifically. This statement is only true if the token contract owner is the same as trustee owner, but this precondition is not enforced anywhere.
We recommend replacing
tokenContract.owner in line 206 to specify the transfer target precisely.
Two unchecked math operations involving constant values
There are some math operations that aren’t checked like calculating the number of bought tokens orthe actual cost for the partial amount of tokens. It’s always better to be safe and perform checked operations.
It’s worth mentioning that the safety of all the other calculations were correctly ensured with the
Consider using the
SafeMath library, or performing pre-condition checks on all of the math operations.
Update: Fixed in this commit by introducing the _
Missing validation of the TOKEN_SALE constant
TokenSale contract, there is validation for most of the configuration constants which is a very good practice. The validation skips checking the correctness of
TOKENS_SALE parameter which can allow creation of a contract that is unable to sell any tokens at all.
We recommend adding the missing validation to the
Update: The Simple Token team indicated that,
TOKENS_SALE is effectively validated by (1) the constructor confirming that all of the token values add up to
TOKENS_MAX and (2)
TokenSale.initialize confirming that the balance of
TokenSale is equal to
TOKENS_SALE. It is true that if
TOKEN_SALE were set to
TokenSaleConfig (and other values adjusted) and if we either transfer
TokenSale or do not transfer anything at all, those validations would pass. However, the team thinks they must surely be allowed to assume that their config contract is correct.
Access modifier conflicting with documentation and function body
onlyOps modifier restricts access to the
processAllocation function, allowing only an account that matches the
opsAddress. Such a behaviour conflicts with the documentation that describes the function as Push model which allows _the owner to transfer tokens to the beneficiary_. Moreover, this is also inconsistent with the function body which contains logic that requires a message sender to be the owner or the admin.
We recommend resolving the conflict by either removing the
onlyOpsrestriction access modifier or updating the documentation and removing irrelevant code fragments lines 167–170) from the function body.
Update: Fixed in this commit by updating comments and removing the unreachable code.
Possible to initiate Ownership Transfer with the null address
initiateOwnershipTransfer allows initiating a transfer passing the
0x0 address as a
_proposedOwner. This action will be ineffective as it’s impossible to call the
completeOwnershipTransfer function from the
0x0address. The null address is also used as a special value to mark that the transfer was completed so it’d be the best if it could be set only by internal invocation.
If the contract could be ownerless a confirmation step needs to be skipped for the
0x0 address. Otherwise, we recommend adding a precondition check preventing initiation of the ownership transfer with the
Update: Fixed in this pull request. The team decided to postpone merging the changes to the master branch as it will require excessive refactor of all the tests.
Duplicated logic in mock contracts
FutureTokenSaleLockBoxMock have almost identical content duplicating the code that implements time manipulation behaviour. Not only increases it the maintenance costs but also brings the risk of updating only one of the contracts and skipping another.
We recommend extracting the common logic into an abstract
Mock contract that will be a base class for both
Update: While the Simple Token team agree with the points, they do not feel that this is a concern that merits changing the code.
Imprecise unlock date documentation
The definition of
FutureTokenSaleLockBox contract (The unlock date is initially six months after
26 weeks. The difference is dependent on an exact
unlockDate and requires complex calendar logic to be precisely calculated.
We recommend documenting the periods in fixed time units such as days or weeks.
Update: Fixed in this commit.
Notes & Additional Information
- Congratulations on writing such a thorough test suite! There are 244 tests in total, with most of them covering the integration of all contracts into the token sale contract.
- There is a typo in
Trustee, it saysacount where it should say account. (Update: fixed in this commit).
- The comparison
ok == trueis redundant as the
okvariable is already a boolean value. The same comment applies to checking the function result by
require(hasUnlockDatePassed() == true).(Update: fixed in this commit).
TOKENS_PER_KETHERduplicate the same calculations. We recommend adding a helper constant
TOKENS_PER_ETHto facilitate calculating the previously mentioned values. (Update: the team decided to leave the code as it is, because the constants are unrelated in general and the similar calculation occurs in the current version only).
initializefunction is described as: …link the sale token with the token contract _while the linking process is executed not in this function but in the constructor. Consider updating the comment to explain that the function is only checking correctness of configuration parameters. (Update: _fixed in this commit).
- Consider turning magic numbers such as “18” and “3” that are repeatably used in
buyTokensfunctions to predefined constants. (Update: fixed in this commit).
- Consider using full
ERC20Interfaceinstead of the
TokenInterfacestub to get the compilation time syntax checks. (Update: fixed in this commit).
- Keep in mind that there is a possible attack vector on the
transferFromfunctionality of ERC20 tokens, describedhere. Consider implementing one of the proposed mitigations, or using the ERC20 implementation from OpenZeppelin which already has one in place. (Update: explanation note added in this pull request).
finalizefunction of the
TokenSalecontract there is a suggestion that: The owner will also need to finalize the token contract so that token transfers are enabled. It is actually the
adminthat needs to
finalizethe token, not the
owner. We recommend to update the documentation or change the permission scheme, so the description is compatible with the documentation. (Update: fixed in this commit).
- The contracts
Pausableare very similar to OpenZeppelin library contracts sharing the same names. Consider avoiding code repetition, which can bring regression problems andintroduce unexpected bugs. We recommend using the standard modules from OpenZeppelin. (Update: the team decided to wait with directly linking to the OpenZeppelin library until it offers full support for the new version of solidity compiler).
- Consider removing the call to the
finalizeInternalfunction from the publicly accessible
buyTokensmethod. It’s safer to keep the critical functionality in the hands of admin/owner than to leave it open to the public. (Update: the team decided to leave the current version as they have enough control by using the
No critical or high severity issues were found. A few medium and lower severity issues were found and explained, along with recommendations on how to fix them. Some changes were proposed to follow best practices and reduce the potential attack surface.
Note that as of the date of publishing, the above review reflects the current understanding of known security patterns as they relate to the Simple Token Sale contracts. We have not reviewed the related Simple Token project. The above should not be construed as investment advice. For general information about smart contract security, check out our thoughts here.