GPA Smart Contract Audit
GPAToken.io engaged New Alchemy to audit the smart contracts used for the Game ICO Platform Coin. The smart contracts implement functionality related to the exchange of in-game assets with out-of-game currencies.
The engagement was technical in nature and focused on identifying security flaws in the design and implementation of the contracts, finding differences between the contracts’ implementation and their behavior as described in public documentation, and finding any other issues with the contracts that may impact their trustworthiness. GPAToken.io provided New Alchemy with access to the relevant source code and whitepapers.
The audit was performed over 3 business days. This document describes the issues discovered in the audit.
The GPA smart contracts consist of 2 contracts, no tests, and a whitepaper. They implement a standard ERC20 token with a freeze/unfreeze feature, and a bulk token distribution function (air drop). Overall they demonstrated adherence to coding standards and utilization of industry standard code.
The audit identified no critical issues.
Re-test v2.0: New Alchemy has discussed the prior audit results with GPAToken.io, advised on efficient and effective mitigation approaches, and inspected the resulting smart contract code provided for re-testing. New Alchemy has concluded:
- All issues have been addressed and the associated risks effectively mitigated.
- The single minor issue labeled ‘Informational’ below is related to short-address attacks which are contentious among developers, the community is trending towards accepting rather than mitigating this risk, and mitigation can only occur at the source outside of the audited contract code. New Alchemy considers GPAToken.io’s approach appropriate and sufficient.
Files Audited
The code reviewed by New Alchemy is in the GitHub repository https://github.com/CMBaek/nodejs_test at commit hash 321aaaac69b66c868861416240ade60bd2a70564.
The specific files making up the complete deployed package include the following:
GPAAirDrop.sol bb53811c3907c6b07d8fd0b314ba7ba17949b11f
GPA.sol 918cff44ac71f0748a7db0d4d1550a0119d1c39bhttps://github.com/OpenZeppelin/openzeppelin-solidity/tree/1.10.0
contracts/ownership/Ownable.sol 182fc6647cf8caab0ddbfa71215828d9d75457b1
contracts/token/ERC20/MintableToken.sol 6582204becc7ed9223fc640ecc3a6247635e7a72
contracts/token/ERC20/StandardBurnableToken.sol f69034aac025598508cfe049a61b98eafb2424a7New Alchemy’s audit was additionally guided by http://gpatoken.io/pdf/GPA_Whitepaper_EN.pdf shasum 6d925343302fe847da6cb8ad92342cc7618c42bc
Re-test v2.0: New Alchemy performed the retest on the GPA.sol and GPAAirDrop.sol contracts located at https://github.com/binaries-dev/gpa-token/tree/1fad50fb7d43a75efa7c2d1afaa51257a79f6ecd/contracts.
General Discussion
The GPA Smart Contracts encompass 2 custom contracts, GPAToken and GPAAirDrop.
GPAToken
The GPAToken contract builds on industry standard OpenZeppelin StandardBurnableToken and MintableToken contracts. GPAToken adds two features,
- The ability for GPA to freeze individual accounts
- The ability for GPA to freeze all transactions, other than those by GPA
These features could be abused to decrease the availability of GPA tokens, thereby increasing the value of any held by GPA.
Though the whitepaper states a token supply limit of 10 billion tokens, the supply is not limited until the owner calls finishMinting(), at which point no more could be minted.
The scope of this audit does not extend to cover GPA’s key management practices, and therefore this audit generally assumes that those practices will be sufficient to prevent the compromise of any addresses controlled by GPA.
Hacks that lead to key disclosure, like the recent Bancor hack, are not uncommon, but they violate the soundness of the reviewed contracts, since all of the reviewed code implicitly assumes that the owner address(es) of each contract are solely controlled by GPA.
In short, the contracts reviewed in this audit are no more secure than GPA’s key management practices, and those practices have not been reviewed by New Alchemy.
GPAAirDrop
The GPAAirDrop contract is a utility aiding the contract owner in distributing their ERC20 compatible tokens to a list of recipients. The GPAAirDrop contract has no special access to the GPA token contract. The same functionality could be implemented in an Ethereum client by issuing multiple transactions, however not as a single transaction or the same gas cost. Therefore, the contract posesses no security risk to the primary GPA token.
Contract Inheritance Diagram
Contract / Whitepaper / Website Token Coherence
This section describes how accurately the values from the whitepaper are implemented in the actual contracts. Contracts should aim to implement as closely as possible the various descriptions found in the whitepaper and website. For this section, the white paper is going to be considered as the primary reference for all values. The discrepancies found between white paper and actual contracts helps users decide the level of trust they can put into the contracts as implemented.
GPAToken.io did not implement in the contract the total token supply documented in the whitepaper. While New Alchemy acknowledges that it is easier for the contract owner to keep some of the values undefined until the contract is actually initiated/started, it creates a strong burden on the token buyer to actually check these values from the blockchain to ensure the owner has minted the amount specified in the whitepaper and called finishMinting() to disallow further token creation.
Critical Issues
No critical issues were found.
Moderate Issues
1. Fixed: Outdated Dependencies
Utilizing third-party, standardized, battle-tested components provides significant benefits in contract security. These components have a known track record and are generally carefully inspected by many people, well tested and quickly patched when vulnerabilities arise. However, many of these benefits are lost when dependencies become outdated or disconnected from their original source.
The package.json file present in the GitHub repository specifically includes the following dependency references:
"openzeppelin-solidity": "1.10.0",Openzeppelin-solidity should be upgraded to 1.12.0 as follows:
"openzeppelin-solidity": "1.12.0",Re-test v2.0: The GPAToken.io team has updated the openzeppelin-solidity version in the package.json file to the most recent version. Thus, this issue has been fully remediated and the associated risk mitigated.
Minor Issues
2. Informational: Lack of Short-Address Attack Protection
Some Ethereum clients may create malformed messages if a user is persuaded to call a method on a contract with an address that is not a full 20 bytes long. In such a “short-address attack”, an attacker generates an address whose last byte is 0x00, then sends the first 19 bytes of that address to a victim. When the victim makes a contract method call, it appends the 19-byte address to msg.data followed by a value. Since the high-order byte of the value is almost certainly 0x00, reading 20 bytes from the expected location of the address in msg.data will result in the correct address. However, the value is then left-shifted by one byte, effectively multiplying it by 256 and potentially causing the victim to transfer a much larger number of tokens than intended. msg.data will be one byte shorter than expected, but due to how the EVM works, reads past its end will just return 0x00.
This attack affects methods where method parameters include an address followed immediately by a value. There are several of these in the InsurePal smart contracts, with a summary of each contract, line number and function listed below.
GPA.sol
85: function transfer(address _to, uint256 _value) public allowedTrade returns (bool)
95: function transferFrom(address _from, address _to, uint256 _value) public allowedTrade returns (bool) GPAAirDrop.sol
19: function execAirDrop(address _tokenAddr, address[] addrList, uint256[] valList) public onlyOwner returns (uint256)
While the root cause of this flaw is buggy serializers and how the EVM works, it can be easily mitigated in contracts. When called externally, an affected method should verify that msg.data.length is at least the minimum length of the method's expected arguments (for instance, msg.data.length for an external call to PermissionedToken.sol::transfer() should be at least 68: 4 for the hash, 32 for the address (including 12 bytes of padding), and 32 for the value; some clients may add additional padding to the end). This can be implemented in a modifier. External calls can be detected in the following ways:
- Compare the first four bytes of
msg.dataagainst the method hash. If they don't match, then the call is internal and no short-address check is necessary. - Avoid creating
publicmethods that may be subject to short-address attacks; instead create onlyexternalmethods that check for short addresses as described above.publicmethods can be simulated by having the external methods callprivateorinternalmethods that perform the actual operations and that do not check for short-address attacks.
Whether or not it is appropriate for contracts to mitigate the short-address attack is a contentious issue among smart-contract developers. Many, including those behind the OpenZeppelin project, have explicitly chosen not to do so. While it is New Alchemy’s position that there is value in protecting users by incorporating low-cost mitigations into likely target functions, GPA would not stand out from the community if they also choose not to do so.
Re-test v2.0: New Alchemy has discussed this issue with the GPA Token team at length. As noted above, this is a contentious issue among smart contract developers, there are clear community trends towards accepting rather than mitigating this vulnerability, and there are external dependencies such as UI and tools that must be considered. GPAToken.io has chosen to accept this risk at this time and consider future mitigation as their application evolves. New Alchemy considers this an entirely reasonable approach.
3. Fixed: GPAAirDrop extra gas usage
As the only use of targetToken is immediately after being set within the same function, this could be a memory variable within the execAirDrop function rather than stored on the blockchain.
Re-test v2.0: The GPAToken.io team has removed the targetToken variable from the source code. By removing the variable the team has effectively mitigated the finding, and as such New Alchemy considers this finding fixed.
4. Fixed: Unhandled return value
GPAAirDrop.sol
28: targetToken.transferFrom(msg.sender, addrList[i], valList[i].mul(10 ** uint256(decimals)));https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20.md states that Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned! but the return value is ignored.
Re-test v2.0: New Alchemy has inspected the updated project repository to confirm the the return values are handled in all use cases. This security issue has been addressed and remediated.
Smart Contract ABI Reference
This section provides the fully-elaborated application binary interface (ABI) for 3 of the primary contracts as seen at runtime. The ABI defines how the contracts may be interacted with while running inside the EVM. The tables make for very easy inspection of top-level specifics including:
- The definitive reference of all exposed functions
- Correct input and output function signatures
- Presence of constructors and fallbacks as expected
- Correct state mutability (pure, view, nonpayable, payable)
- Extraneous functions which should be private
contracts/GPA ABI Reference
contracts/GPAAirDrop.sol:GPAAirDrop ABI Reference
contracts/GPAAirDrop.sol:ERC20Interface ABI Reference
Line by line comments
This section lists comments on design decisions and code quality made by New Alchemy during the review. They are not known to represent security flaws.
A. Specific Compiler Pragma
contracts/GPA.sol Line 1
contracts/GPAAirDrop.sol Line 1
New Alchemy recommends using the pragma annotation to specify a single target compiler version. This helps more clearly elaborate dependencies, isolates the effects of changing tool versions, and insures the code compiles as intended. As of July 2018 the correct statement should target the latest stable compiler: pragma solidity 0.4.24.
Re-test v2.0: New Alchemy has confirmed that the source files now use the pragma keyword when defining compiler versions. This issue has been fixed.
B. Function ordering
contracts/GPA.sol Lines 38, 48, 55, 62, 69, 76, 85, 95
contracts/GPAAirDrop.sol Line 44
Solidity coding style convention prefers a particular order of function declarations: constructor, fallback function (if it exists), external, public, internal then private. Within a grouping, the view and pure functions are placed last. The line numbers referenced above are internal functions that should be ideally moved after all of the public function declarations.
Re-test v2.0: New Alchemy has verified that the source code functions have been reorganized in a manner which follow the Solidity coding conventions. This issue has been fixed.
C. State-changing functions do not emit events.
contracts/GPA.sol Lines 38, 48, 69, 76, 85, 95
contracts/GPAAirDrop.sol Lines 19, 36, 44
The common security mantra of “deter, detect, delay and respond” provides a useful mental model for consideration during smart contract development. The ‘detect’ and ‘respond’ aspects are very dependent upon logging and auditing functionality. For these reasons, New Alchemy recommends emitting events whenever significant state-changing functionality is invoked (and further differentiating between success and failure) so auditing can be performed without a difficult direct inspection of the blockchain.
Re-test v2.0: New Alchemy has inspected the source code and confirms that all state changing functions now emit events. The added events allow for more complete logging. Thus, the issue has been fixed.
D. Redundant inheritance of Ownable
contracts/GPA.sol Line 11
GPAToken inherites Ownable twice, as the inherited MintableToken also inherits Ownable. Removing unused code generally reduces unintended consequences thereby increasing security.
Re-test v2.0: New Alchemy has confirmed that the redundant Ownable contract inhericance has been removed from the source code. The GPA.sol file now only inherits the StandardBurnableToken and MintableToken contracts, fixing this issue.
Disclaimer
The audit makes no statements or warranties about utility of the code, safety of the code, compiler or tools stability, suitability of the business model, regulatory regime for the business model, or any other statements about fitness of the contracts to purpose, or their bug-free status. The audit documentation is for discussion purposes only.
New Alchemy is a leading blockchain strategy and technology group specializing in tokenized capital solutions for the most innovative companies worldwide. New Alchemy’s Blockchain Security division is a highly trusted name that has assisted clients in securely raising over $500m through custom-tailored solutions, smart contract audits and comprehensive security strategy. Get in touch with us at Hello@NewAlchemy.io
