We’ve been asked by the Golem team to review and audit their new token code. We looked at their contracts and now publish our results.
The audited contracts are at their golem-crowdfunding GitHub repo. The version used for this report is commit 50100b27a7c6841ed430a028d100f5d45ba08fb1. The main contract file is Token.sol.
Here’s our assessment and recommendations, in order of importance:
We have not found any severe security problems with the code.
There’s a problem with using timestamps and now (alias for block.timestamp) for contract logic, based on the fact that miners can perform some manipulation. In general, it’s better not to rely on timestamps for contract logic. The solutions is to use block.number instead, and approximate dates with expected block heights and time periods with expected block amounts.
The GNTAllocation contract uses timestamps at several points. The risk of miner manipulation, though, is really low. The potential damage is also limited: miners could only slightly manipulate the developer lock period duration. We recommend the team to consider the potential risk and switch to block.number if necessary.
For more info on this topic, see this stack exchange question.
block.number is correctly used in the Token.sol file.
There are many unchecked math operations in the code. We couldn’t find any related attack vectors on the GNT contract, but it’s always better to be safe and perform checked operations. Consider using a safe math library, or performing pre-condition checks on any math operation.
The fact that GNT supply is limited to 820,000 ether (and thus at most 820,000,000 GNT assuming all tokens are created at the best possible price) helps prevent possible overflows.
Use of send is always risky and should be analyzed in detail. Two occurrences found in line 185 of Token.sol and line 214 of Token.sol.
– Always check send return value: OK.
– Consider calling send at the end of the function: Risk is low because in one case, the contract sends to golemFactory, which is a trusted contract, and almost at the end of the function. Consider refactoring the code to have send at the end, though.
– Favor pull payments over push payments: No problems with push payment used, because golemFactory will be controlled by the Golem team. Bear in mind that if send at line 185 fails for any reason, the whole finalize call will fail.
Duplicate code makes it harder to understand the code’s intention and thus, auditing the code correctly. It also increases the risk of introducing hidden bugs when modifying one of the copies of some code and not the others. We recommend the following to remove duplicate code:
createTokens function.Formal security audits are not enough to be safe. We recommend implementing an automated contract-based bug bounty and setting a period of time where security researchers from around the globe can try to break the token’s invariants. For more info on how to implement automated bug bounties with OpenZeppelin, see this guide.
migrationAgent address has a lot of power. Private key for that address should be stored with high security standards.No severe security issues were found. Some changes were recommended to follow best practices and reduce potential attack surface. Overall code quality is very good! Functions are short, and all start with well-defined and well-ordered preconditions. In general, external effects are left to the end of functions. Most of the code is very well commented. 👍
Note that as of the date of publishing, the above review reflects the current understanding of known security patterns as they relate to GNT token contract. We have not reviewed the related Golem project. The above should not be construed as investment advice or an offering of GNT. For general information about smart contract security, check out our thoughts here.