Bug Hunting with Crytic

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 issue we discovered. Read on for where we found them:

1. Unused return value can allow free token minting
2. registerVerificationKey always returns empty bytes
3. MerkleTree.treeHeight and MerkleTree.zero can be constant
4. Modifiers can return the default value
5. Dangerous strict equality allows the contract to be trapped
6. ABI encodePacked collision
7. Missing inheritance is error-prone
8. Msg.value is used two times in fundWithAward
9. Reentrancy might lead to theft of tokens
10. [Pending notification]
11. [Pending notification]
12. [Pending notification]

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:

• Classifieds.cancelTrade(uint256)
• Classifieds.executeTrade(uint256) 

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.