Cally contest - shenwilly'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#L281-L289

Vulnerability details

Impact

If protocol feeRate is increased when there are active options, vault creators will receive lesser amount of eth than expected when creating them. In extreme case, a malicious/compromised owner can frontran an exercise transaction and steal all funds meant for the vault creator by setting the fees to 100%.

Proof of Concept

• feeRate is set to 0.
• Alice creates a vault, setting 100 eth as reserve price.
• Bob buys the option from Alice.
• Bob wants to exercise, sending 100 eth as msg.value.
• Malicious owner frontran the exercise transaction by setting the feeRate to 1e18.
• Owner gets 100 eth, Alice gets nothing.

Recommended Mitigation Steps

Add a feeRate parameter in Vault, and store the current feeRate when buyOption is called. Use the stored feeRate for the fee calculation when the option is exercised.

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

Missing sanity check in setFeeRate There is no input validation in setFeeRate. A faulty payload could set the feeRate to a very high amount, which would cause problems when options are exercised:

Loss of fund for vault creators if feeRate is near 1e18, as the fund is fully transferred to protocol, or Buyers unable to exercise if feeRate is higher than 1e18, as fee would be higher than msg.value. Recommended Mitigation Add sanity checks when setting feeRate, such as:

require(feeRate_ <= 3e17, "Fee must not be higher than 30%");

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

Incompatability with deflationary / fee-on-transfer tokens Function Cally.createVault function takes a tokenIdOrAmount parameter but this parameter is not the actual transferred amount for fee-on-transfer / deflationary (or other rebasing) tokens in case tokenType = ERC20 Impact The actual deposited amount might be lower than the specified depositAmount of the function parameter. And when users exercise or withdraw they not only receive less than expected amount but also take funds of other vaults with the same vault.token too, causes loss of funds. Proof-of-concept https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L200 https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L296 https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L345 Recommended Mitigation Steps Transfer the tokens first and compare pre-/after token balances to compute the actual amount.

Lines of code

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

Vulnerability details

Impact

When exercising options, transferFrom is used to transfer the NFT to the caller. If the caller is a contract that relies on ERC721TokenReceiver standard and not aware of incoming ERC721 tokens, the NFT could be locked.

Recommended Mitigation Steps

Consider using safeTransferFrom over transferFrom when exercising.

Low Risk Vulnerabilities

1. Missing sanity check in setFeeRate

There is no input validation in setFeeRate. A faulty payload could set the feeRate to a very high amount, which would cause problems when options are exercised:

• Loss of fund for vault creators if feeRate is near 1e18, as the fund is fully transferred to protocol, or
• Buyers unable to exercise if feeRate is higher than 1e18, as fee would be higher than msg.value.

Recommended Mitigation

Add sanity checks when setting feeRate, such as:

require(feeRate_ <= 3e17, "Fee must not be higher than 30%");

2. Missing sanity check on durationDays

When creating vault, there is no check to make sure that durationDays is not too big. A faulty frontend could mistakenly set it to an unreasonable amount (years).

Recommended Mitigation

Add sanity checks when setting durationDays, such as limiting it to one year:

require(durationDays <= 365, "durationDays too big");

3. Buyers can send more premium than needed

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

When buying options, the function only checks that msg.value must not be lower than premium. A faulty frontend can set a higher amount and the transaction would still succeed.

Recommended Mitigation

Unless there's a good reason to allow sending more premium than necessary, update the statement to equality check:

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

4. Possible redundant InitiatedWithdrawal event emission

There's no check to prevent initiateWithdraw from being called repeatedly. InitiatedWithdrawal event can keep being emitted, which could disrupt off-chain analytics that track this particular event.

Recommended Mitigation

Add a check to make sure it's only callable once per vault:

require(!_vaults[vaultId].isWithdrawing, "Vault withdrawal is already in progress"); 

5. Wrong requirement message

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

The check in Cally.sol L169 states that "Reserve strike too small" when the check fails. It should be "Reserve strike too high".