Crytic, our Github app for discovering smart contract flaws, is kind of a big deal: It detects security issues without human intervention, providing continuous assurance while you work and securing your codebase before deployment.
Crytic finds many bugs no other tools can detect, including some that are not widely known. Right now, Crytic has 90+ detectors, and we’re continuously adding new checks and improving existing ones. It runs these bug and optimization detectors on every commit, and evaluates custom security properties that you add yourself too!
Today, we’re sharing twelve issues in major projects that were found solely by Crytic, including some of high severity.
Here are the issues we discovered. Read on for where we found them:
- Unused return value can allow free token minting
- registerVerificationKey always returns empty bytes
- MerkleTree.treeHeight and MerkleTree.zero can be constant
- Modifiers can return the default value
- Dangerous strict equality allows the contract to be trapped
- ABI encodePacked collision
- Missing inheritance is error-prone
- Msg.value is used two times in fundWithAward
- Reentrancy might lead to theft of tokens
- [Pending acknowledgement]
- [Pending acknowledgement]
- [Pending acknowledgement]
Ernst & Young Nightfall
Crytic found three bugs in E&Y’s Nightfall project, including one critical vulnerability that could allow anyone to mint free tokens.
Issue 1: Unused return value can allow minting free tokens
Description
FTokenShield.mint does not check the return value of transferFrom. If FTokenShield is used with a token that does not revert in case of incorrect transfers, and only returns false (e.g., BAT), anyone can mint free commitments—and an attacker can mint tokens for free.
A similar issue with less impact occurs in FTokenShield.burn: here fToken.transfer is not checked.
Recommendation
Check the return value of transferFrom and transfer.
Crytic report
Issue 2: registerVerificationKey always returns empty bytes
Description
FTokenShield.registerVerificationKey and NFTokenShield.registerVerificationKey return an empty bytes32. It is unclear what the correct returned value should be. This might lead to unexpected behavior for third parties and contracts.
Recommendation
Consider either returning a value, or removing the return value from the signature.
Crytic report
Issue 3: MerkleTree.treeHeight and MerkleTree.zero can be constant
Description
treeHeight and zero can be declared constant in MerkleTree.sol to allow the compiler to optimize this code.
Recommendation
State variables that never change can be declared constant to save gas.
Crytic report
DeFiStrategies
Crytic found one unusual issue in DeFiStrategies: The lack of placeholder execution in a modifier leads the caller function to return the default value. Additionally, Crytic found an issue related to strict equality on the return of a balanceOf call.
Issue 4: Modifiers can return the default value
Description
The SuperSaverZap.onlyInEmergency() and SuperSaverZap.stopInEmergency() modifiers do not revert in case of invalid access. If a modifier does not execute or revert, the execution of the function will return the default value, which can be misleading for the caller.
Recommendation
Replace the if() condition by a require in both modifiers.
Crytic report
Issue 5: Dangerous strict equality allows the contract to be trapped
Description
ERC20toUniPoolZapV1_General.addLiquidity has strict equality on the _UniSwapExchangeContractAddress balance. This behavior might allow an attacker to trap the contract by sending tokens to _UniSwapExchangeContractAddress.
Recommendation
Change == to <= in the comparison.
Crytic report
DOSNetwork
Crytic found another issue that is not well known in DOSNetwork: If abi.encodedPacked is called with multiple dynamic arguments, it can return the same value with different arguments.
Issue 6: ABI encodePacked Collision
Description
DOSProxy uses the encodePacked Solidity function with two consecutive strings (dataSource and selector): eth-contracts/contracts/DOSProxy.sol.
This schema is vulnerable to a collision, where two calls with a different dataSource and selector can result in the same queryId (see the Solidity documentation for more information).
Recommendation
Do not use more than one dynamic type in encodePacked, and consider hashing both dataSource and selector with keccak256 first.
Crytic Report
ethereum-oasis/Baseline
Crytic found an architectural issue in the Baseline Protocol, among others: A contract implementing an interface did not inherit from it.
Issue 7: Missing inheritance is error-prone
Description
Shield is an implementation of ERC1820ImplementerInterface, but it does not inherit from the interface. This behavior is error-prone and might prevent the implementation or the interface from updating correctly.
Recommendation
Inherit Shield from ERC1820ImplementerInterface.
Crytic report
EthKids
Crytic found another unusual issue in EthKids: this.balance includes the amount of the current transaction (msg.value), which might lead to incorrect value computation.
Issue 8: Msg.value is used two times in fundWithAward
Description
The use of this.balance in fundWithAward does not account for the value already added by msg.value. As a result, the price computation is incorrect.
fundWithAward computes the token amount by calling calculateReward with msg.value:
function fundWithAward(address payable _donor) public payable onlyWhitelisted {
uint256 _tokenAmount = calculateReward(msg.value, _donor);
calculateReward calls calculatePurchaseReturn, where _reserveBalance is this.balance and _depositAmount is msg.value:
return bondingCurveFormula.calculatePurchaseReturn(_tokenSupply, _tokenBalance, address(this).balance, _ethAmount);
In calculatePurchaseReturnn, baseN is then computed by adding _depositAmount (msg.value) to _reserveAmount (this.balance):
uint256 baseN = _depositAmount.add(_reserveBalance);
msg.value is already present in this.balance. For example, if this.balance is 10 ether before the transaction, and msg.value is 1 eth, this.balance will be 11 ether during the transaction). As a result, msg.value is used two times, and calculatePurchaseReturn is incorrectly computed.
Recommendation
Change the price computation so that _reserveBalance does not include the amount sent in the transaction.
Crytic report
HQ20
Finally, Crytic found a well-known issue in HQ20: reentrancy. This reentrancy is interesting as it occurs if the contract is used with a token with callback capabilities (such as ERC777). This is similar to the recent uniswap and lendf.me hacks.
Issue 9: Reentrancy might lead to theft of tokens
Description
Classifieds calls transferFrom on external tokens without following the check-effects-interaction pattern. This leads to reentrancy that can be exploited by an attacker if the destination token has a callback mechanism (e.g., an ERC777 token).
There are two methods with reentrancy issues:
Recommendation
Follow the check-after-effect pattern.
Crytic report
Start using Crytic today!
Crytic can save your codebase from critical flaws and help you design safer code. What’s not to like?
Sign up for Crytic today. Got questions? Join our Slack channel (#crytic) or follow @CryticCi on Twitter.
Article Link: https://blog.trailofbits.com/2020/05/15/bug-hunting-with-crytic/