Cally contest - hickuphh3's results

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L238

Vulnerability details

Details & Impact

vault.durationDays is of type uint8, thus allowing a maximum value of 255. 1 days = 86400, thus fitting into a uint24. Solc creates a temporary variable to hold the result of the intermittent multiplication vault.durationDays * 1 days using the data type of the larger operand.

In this case, the intermittent data type used would be uint24, which has a maximum value of 2**24 - 1 = 16777215. The maximum number allowable before overflow achieved is therefore (2**24 - 1) / 86400 = 194.

Proof of Concept

Insert this test case into BuyOption.t.sol

function testCannotBuyDueToOverflow() public {
  vm.startPrank(babe);
  bayc.mint(babe, 2);
  // duration of 195 days
  vaultId = c.createVault(2, address(bayc), premiumIndex, 195, strikeIndex, 0, Cally.TokenType.ERC721);
  vm.stopPrank();

  vm.expectRevert(stdError.arithmeticError);
  c.buyOption{value: premium}(vaultId);
}

Then run

forge test --match-contract TestBuyOption --match-test testCannotBuyDueToOverflow

Tidbit

This was the 1 high-severity bug that I wanted to mention at the end of the C4 TrustX showcase but unfortunately could not due to a lack of time :( It can be found in the vulnerable lottery contract on L39. Credits to Pauliax / Thunder for the recommendation and raising awareness of this bug =p

Reference

Article

Recommended Mitigation Steps

Cast the multiplication into uint32.

vault.currentExpiration = uint32(block.timestamp) + uint32(vault.durationDays) * 1 days;

Judge has assessed an item in Issue #41 as Medium risk. The relevant finding follows:

L02: Lack of upper bound for feeRate Line References https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L120

Description Fees can be set above 1e18, preventing options from being exercised.

Recommended Mitigation Steps Consider having a hard cap of x% < 100%.

// Eg. cap protocol fee to max 5% require(feeRate_ < 5e16, "feeRate limit exceeded");

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L200

Vulnerability details

Details & Impact

In addition to ERC721s, ERC20 based call options are also supported. There are some ERC20 tokens that charge a fee (FoT tokens) for every transfer() / transferFrom() invocation.

The contract assumes that the amount receivable from the call option creation is equivalent to the vault.tokenIdOrAmount in createVault(), which isn’t necessarily the case. This will affect token transfers for option exercises and withdrawals, where the actual balance held by the contract could be less than vault.tokenIdOrAmount.

Recommended Mitigation Steps

One possible mitigation is to measure the asset change right before and after the safeTransferFrom() invocation in createVault().

if (vault.tokenType == TokenType.ERC721) {
  ERC721(vault.token).transferFrom(msg.sender, address(this), vault.tokenIdOrAmount)
} else {
  uint256 balanceBefore = ERC20(vault.token).balanceOf(address(this));
  ERC20(vault.token).safeTransferFrom(msg.sender, address(this), vault.tokenIdOrAmount);
  _vaults[vaultId].tokenIdOrAmount = ERC20(vault.token).balanceOf(address(this)) - balanceBefore;
}

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L199 https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L295 https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L344

Vulnerability details

Details & Impact

The transferFrom() method is used instead of safeTransferFrom(), presumably to save gas. I however argue that this isn’t recommended because:

• OpenZeppelin’s documentation discourages the use of transferFrom(), use safeTransferFrom() whenever possible
• Given that any NFT can be used for the call option, there are a few NFTs (here’s an example) that have logic in the onERC721Received() function, which is only triggered in the safeTransferFrom() function and not in transferFrom()

Recommended Mitigation Steps

Call the safeTransferFrom() method instead of transferFrom() for NFT transfers. Note that the CallyNft contract should inherit the ERC721TokenReceiver contract as a consequence.

abstract contract CallyNft is ERC721("Cally", "CALL"), ERC721TokenReceiver {...}

Low Severity Findings

L01: Implicit upper bound for call option

Line References

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L77

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L162

Description

durationDays is of type uint8, thus having a maximum value of 255. I’m unsure if it’s a bug or feature, but it would prevent creating a year-long call option.

Recommended Mitigation Steps

Perhaps increase it to type uint16. A maximum value of 65535 days ~= 170 years seems more than sufficient for a call option duration.

L02: Lack of upper bound for feeRate

Line References

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L120

Description

Fees can be set above 1e18, preventing options from being exercised.

Recommended Mitigation Steps

Consider having a hard cap of x% < 100%.

// Eg. cap protocol fee to max 5%
require(feeRate_ < 5e16, "feeRate limit exceeded");

L03: Incorrect revert reason

Line References

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L169

Description

The revert reason for checking that the dutchAuctionReserveStrike is smaller than the starting price is "Reserve strike too small", when it should be "Reserve strike too large".

Recommended Mitigation Steps

require(dutchAuctionReserveStrike < strikeOptions[dutchAuctionStartingStrikeIndex], "Reserve strike too large");

L04: Restrict msg.value to be strictly equal to calculated premium

Line References

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L224

Description

There isn’t a reason why a user should pay more than the calculated premium. It would therefore be better to ensure strict equality of msg.value and the premium in buyOption().

Recommended Mitigation Steps

require(msg.value == premium, "Incorrect ETH amount sent");

Non-Critical Findings

NC01: Mention that dutchAuctionReserveStrike value is in ether wei

Line References

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L156

Description

For better clarity, it would be good to state the value is in ether wei.

Recommended Mitigation Steps

/// @param dutchAuctionReserveStrike The reserve strike (in ether wei) for each dutch auction

Preface

Gas optimizations have been benchmarked against the current configuration. This can be done by running

forge snapshot

then, after modifications, run

forge snapshot --diff

G01: Increase solc runs

Description

The number of solc runs used for contract compilation is 1000. This number can be bumped significantly to produce more gas efficient code (max value of 2**32-1). More information can be found in the solidity docs.

Gas Saved

6 to 528

Recommended Mitigation Steps

The maximum number of runs attainable before the max contract size is exceeded is about 5000. While deployment costs will increase, functions will cost less gas to execute.

[default]
solc_version = "0.8.13"
fuzz_runs = 1000
optimizer_runs = 5000
optimizer = true

G02: Redundant TokenType check

Line References

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L171

Description

The type check on the enum TokenType is redundant because Solidity implictly enforces this check.

Gas Saved

66

Recommended Mitigation Steps

Remove the check.

G03: Redundant null ownerOf check

Line References

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L214

Description

The null ownerOf is checked in Solmate already, making this a duplicate check.

Gas Saved

253

Recommended Mitigation Steps

Remove the check. Alternatively, consider overriding the ownerOf() method to remove the null check in Solmate because a number of instances check ownerOf() against msg.sender which is guaranteed to be non-null.

G04: Dutch auction price calculation can be unchecked

Line References

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L417-L419

Description

The dutch auction price calculation can be unchecked because the variables are bounded, where, when the function is called internally, the following are true:

• auctionEndTimestamp - block.timestamp is at most AUCTION_DURATION = 86400
• startingStrike is a value from the strikeOptions array, which contains a Fibonacci sequence of ether values up to 6765 ether.

Therefore, we can be sure that there are no overflow in the calculations:

• 0 ≤ delta ≤ 86400

uint256 progress = (1e18 * delta) / AUCTION_DURATION;

means the largest value of the intermediate calculation is 1e18 * 86400

• 0 ≤ progress ≤ 1e18

uint256 auctionStrike = (progress * progress * startingStrike) / (1e18 * 1e18);

means the largest value of the intermediate calculation is 1e18 * 1e18 * startingStrike = 1e18 * 1e18 * 6765 * 1e18 = 6.765e+57 which is within type(uint256).max

• 0 ≤ auctionStrike ≤ startingStrike = 6765 ether = 6765 * 1e18

The assumptions made might of course not hold when called externally. Nevertheless, the gas savings seem substantial enough to consider implementing the change.

Gas Saved

382 or 421

Recommended Mitigation Steps

uint256 delta;
uint256 auctionStrike;
unchecked {
  if (auctionEndTimestamp > block.timestamp) delta = auctionEndTimestamp - block.timestamp;
  uint256 progress = (1e18 * delta) / AUCTION_DURATION;
  auctionStrike = (progress * progress * startingStrike) / (1e18 * 1e18);
}

NG01: Use of IR Optimizer

Description

The new Yul-based optimizer that is shipped with 0.8.13 ”can do much higher-level optimization across functions”. However, when I turned it on together with the increase in solc runs, an increase in gas usage is observed instead.

The optimizer can be turned on by setting via_ir = true in the foundry.toml file.