Loss of asset due to improperly setting dutchAuctionReserveStriked

[code423n4]

Lines of code

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

Vulnerability details

The Option writer (Victim) believes they will be writing a call option, but ends up selling their asset at a discount.

Proof of Concept

If dutchAuctionReserveStrike is improperly set, the writer is effectively writing a call option that is immediately in the money.

• Alice creates a vault for an NFT with spot value = 5 ETH using the following parameters
  • a dutchAuctionStartingStrikeIndex of 3, corresponding to 8 ETH starting price
  • A premiumIndex of 4, corresponding to 0.1 ETH premium
  • a dutchAuctionReserveStrike of 2 ETH
• As the dutch auction reaches its halfway point, the current Dutch auction strike will be equal to the NFT spot price, and any further reduction will sell at a discount
• The dutch auction proceeds over its 24hr period without a bid. This can be due to any arbitrary reason such as lack of awareness of the Cally protocol.
• At the end of the auction period, the price returned by getDutchAuctionStrike will return dutchAuctionReserveStrike = 1 ETH
• Another user Bob calls buyOption, paying the 0.1 ETH premium, which will set the vault's currentStrike = 2 ETH
• Bob immediately exercises the option, paying 2 ETH
• Bob has obtained an NFT worth ~5 ETH for 2.1 ETH

Mitigations

• The frontend should warn users if their dutchAuctionReserveStrike is below the collection floor
• Alternately, consider implementing a robust oracle solution which will not allow the creation of vaults with a dutchAuctionReserveStrike below floor.

[outdoteth]

While this is technically correct, this is intended behaviour. It's up to the user to select the parameters and adjust accordingly. an onchain oracle introduces another large attack vector and limits the protocol to assets that have a reliable oracle.

we also cannot appraise the value of tail-end nfts so there is no robust way to prevent a user making this mistake even on the frontend. disputed because the "risks" are very clear to anyone who is using the platform. risks are in quotes because it is not a risk but an essential mechanism to get a guarantee fill via arbitrage.

[HardlyDifficult]

This appears to be by design. It's a fair point that the frontend should be very clear. There may be a valid use case here since the NFT market is very volatile as well. Additionally the original owner should be able to buy the option in this scenario to save their NFT.