Cally contest - TomFrenchBlockchain's results

Lines of code

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

Vulnerability details

Impact

Owner of Cally contract may DOS exercising any options, allowing options to be sold which will never be redeemable.

Proof of Concept

The owner of the Cally contract may set feeRate to arbitrary values, including in excess of 1e18:

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

If feeRate > 1e18 then L289 in the snippet linked below will revert, preventing any options from being exercised:

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

In the case where the owner of the Cally contract has a conflict of interest such as holding vaults for which options will soon become in-the-money, they can DOS to prevent these from being exercised until they can close those vaults.

Recommended Mitigation Steps

Choose a sensible maximum platform fee (well below 100%) and revert someone attempts to set it above that value. This will ensure that even in the case of compromised ownership over the Cally contract all options will work as intended.

List of contents:

• Vault struct may be packed further
• feeRate and protocolUnclaimedFees may be packed in storage
• Unnecessary bounds checking of provided enum values
• Use bitwise and instead of modulo 2
• Some arithmetic can be made unchecked

Vault struct may be packed further

The currentStrike and dutchAuctionReserveStrike fields of the Vault struct current use a full slot each

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

However we know that the greatest value which these can take is 6765e18:

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

These will both fit in a uint128 allowing them to fit in the same slot.

feeRate and protocolUnclaimedFees may be packed in storage

As we know that feeRate is bounded above by 1e18, it will fit in a uint64, This would leave 192bits for protocolUnclaimedFees - enough for 6.2 * 10^39 Ether - we can then safely pack these values together in the same slot.

Unnecessary bounds checking of provided enum values

On this line we check that the provided enum value is any of the valid enum values for TokenType:

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

This is unnecessary as if an invalid value for the enum were passed, this would be automatically picked up by solidity and fail before we even reach this line of code.

See this finding: https://github.com/code-423n4/2021-06-realitycards-findings/issues/9

Use bitwise and instead of modulo 2

We often check if a tokenId is odd or even to see if a user is interacting with an option or a vault. As we only need to check the final bit to determine this we don't need all the overhead of the modulo operator.

We can instead just use a bitwise and with 1 (x & 1) to perform the same check for cheaper.

See: https://twitter.com/clemlak/status/1521973218864771073?cxt=HHwWgsC9wYWlkZ8qAAAA

Some arithmetic can be made unchecked

In a number of places we perform checked arithmetic where we know the result cannot over/underflow.

vault.durationDays <= 256 so this won't overflow until about 80 years time. https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L238

No vault with tokenId == 0 exists: https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L265 https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L459

Will not overflow unless protocol fee is above 100% (which should be prevented): https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L289

The multiplications here will fit nicely in a uint256 for even the largest strike price so checking for overflows is unnecessary: https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L417-L419