Cally contest - crispymangoes's results

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L119-L121%20%60%60%60solidity%20function%20setFee(uint256%20feeRate_)%20external%20onlyOwner%20{%20feeRate%20=%20feeRate_;%20}%20%60%60%60

Vulnerability details

Impact

setFee has no require statement enforcing a maximum. This means that if the owner were to act malicous, or if the owners private key were compromised, they'd be able to set the fee to 100% whenever they wanted to. This issue is magnified because of the nature of how options work. Since the owner can see when options are expiring, and whether they are in the money or not, they can have a pretty good idea about when someone would try to exercise an option.

Proof of Concept

Imagine the following scenario:

-Marry creates a vault with her BAYC -Alice buys Marry's call option. -Some time passes. -Bob(the owner) sees that Alice is holding an option for a BAYC, that is currently in the money with only 1 day left till expiration. -Bob knows it is very likely that Alice will exercise the option soon, so he calls setFee and raises the fee from 1% -> 100%. -Alice exercises the option sending 125 ETH with her call to exercise. -Alice gets Marry's NFT, but Marry does not get any of the sale ETH, instead Bob has taken all of it. -Bob calls withdrawProtocolFees and claims the 125 ETH.

Tools Used

-None

Recommended Mitigation Steps

-Add a reasonable max fee require statement like this

    function setFee(uint256 feeRate_) external onlyOwner {
        require( feeRate_ <= MAX_FEE, "feeRate_ too big");
        feeRate = feeRate_;
    }

-This would still allow the owner to change the fee from 1% -> 5% (or whatever the max fee is set to), but atleast vault creators now know that at most the owner can only take whatever the max fee is.