Cally contest - dipp's results

Lines of code

Cally.sol#L224

Vulnerability details

Impact

When buying an option, the msg.value of the call to the buyOption function may be larger than the actual value of the premium.

This could lead to users accidently sending more than the cost of the premium and losing the extra ETH.

Proof of Concept

1. User calls buyOption for a vault with a 0.1 ETH premium.
2. User sets the msg.value for the call to 1 ETH.
3. The condition on line 224 is passed.
4. The user loses 0.9 ETH

Recommended Mitigation Steps

Consider setting the condition on line 224 to require(msg.value == premium, "Incorrect ETH amount sent").

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

No min/max fee rate Line Refrences Cally.sol#L119-121

Description Limits for the fee rate should be set to avoid mistakes when setting the fee. A fee rate that is over 100% could result in the exercise function not being able to execute since the computation on line 289 would result in an underflow and revert.

A constant variable for the max fee rate could be used. The setFee function should check the supplied feeRate against this max fee value.

Lines of code

Cally.sol#L158-L201 Cally.sol#L258-L297 Cally.sol#L318-L346

Vulnerability details

Impact

If the underlying asset for a vault is a fee-on-transfer ERC20 token then the actual amount received by the contract could be less than the value of tokenIdOrAmount of the vault.

If the contract does not have enough tokens to cover the difference then the exercise and withdraw functions would not work since the safeTransfer function at the end of exercise and withdraw would revert due to an insufficient balance.

This could lead to vault owners being unable to access their funds until the contract has enough tokens and option holders might be unable to exercise their option before it expires, leading to lost funds.

Proof of Concept

Unable to withdraw:

1. Create a vault using a fee-on-transfer token as the underlying asset. For this instance, the Cally.sol contract holds none of these tokens before the initial transfer into the contract.
2. At the end of createVault, the amount of vault.tokenIdOrAmount tokens is transferred from the owner of the vault to the Cally.sol contract.
3. After the transfer, the contract has a balance less than vault.tokenIdOrAmount of the tokens.
4. When trying to withdraw the tokens, after calling initiateWithdraw, the withdraw function reverts when the safeTransfer function is called.

Unable to exercise:

1. Buy an option from a vault with fee-on-transfer ERC20 tokens as the underlying asset.
2. When calling exercise, the safeTransfer at the end of the function will fail if the contract does not have enough tokens. The option holder will not be able to exercise the option.

Recommended Mitigation Steps

Consider checking the contract's balance before and after ERC20 transfers in createVault. The vault.tokenIdOrAmount could be set to the difference of the balances.

Lines of code

Cally.sol#L295

Vulnerability details

Impact

The transferFrom function is used in the createVault, withdraw and the exercise functions to transfer the underlying ERC721 vault asset. transferFrom sends an ERC721 asset to a receiver regardless of whether it is able to receive it or not.

In the exercise function, when msg.sender is a contract, the transferFrom function could send the underlying ERC721 asset to a contract that cannot interact with an ERC721, effectively locking it.

Recommended Mitigation Steps

Consider using safeTransferFrom instead of transferFrom in the exercise function. This will revert if the onERC721Received function is not implemented in the receiving contract, not allowing an incompatible contract to exercise the option.

Incompatible contracts would still be able to buy options but would at least not waste more ETH when trying to exercise the option.

1: Incorrect error message

Line Refrences

Cally.sol#L169

Description

The condition only passes if the reserve strike is less than the starting strike. So if it fails then the reserve strike is too big and the error message should display something like "Reserve strike too large" or "Starting strike too small."

2: No min/max fee rate

Line Refrences

Cally.sol#L119-121

Description

Limits for the fee rate should be set to avoid mistakes when setting the fee. A fee rate that is over 100% could result in the exercise function not being able to execute since the computation on line 289 would result in an underflow and revert.

A constant variable for the max fee rate could be used. The setFee function should check the supplied feeRate against this max fee value.