setFee() function lacks validation, can lead to less funds than expected for vault beneficiary

[code423n4]

Lines of code

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

Vulnerability details

Impact

The setFee() function is used to set the protocol fee that is applied when a buyer exercises a call. This setter function does not apply any validation, meaning the feeRate_ parameter may be accidently or maliciously set to a value that leads to unintended consequences. Also, changes to the feeRate are applied to all future calls to exercise() which can be somewhat misleading to vault owners that deposited their ERC721/ERC20 while a lower fee was active.

Unwanted Result #1: The feeRate is set higher than 1e18, buyer will not be able to exercise the call.

If the feeRate is set to a value higher than 1e18, a call to exercise() will fail every time. This is because fee will be larger than msg.value via the following formula:

fee = (msg.value * feeRate) / 1e18;

Failing on underflow here:

ethBalance[getVaultBeneficiary(vaultId)] += msg.value - fee;

Motivation: This value could be accidently set too high or a malicious owner could intentionally set it too high in order to DOS the buyers from exercising calls. The potential motivation would be to let the calls expire to hopefully buy the call at a lower price.

Unwanted Result #2: Fee intentionally set higher than advertised right before exercise call.

Motivation: A malicious owner is able to advertise a low or 0 feeRate to attract users to deposit their ERC721s or ERC20s into Cally. At any point, the owner is able to set the feeRate higher. Most diabolically, the owner will set the feeRate higher right after a call is bought. At this point, the vault owner does not have any control over their deposited ERC721/ERC20. The owner increases the fee potentially as high as 100%, the call is exercised, all proceeds go to Cally.

This most likely is not intended from the Cally team, but users may view this possibility as a risk and choose not to deposit into Cally.

Tools Used

Manual review

Recommended Mitigation Steps

Firstly, I recommend adding thresholds for the min and max values that feeRate is able to be set within. This will provide validation against accidental or intended changes to feeRate that are not in the best interests of vault owners.

Secondly, changes to feeRate are applied immediately despite when the vault owner deposited their ERC721/ERC20. Perhaps it would be more fair to users if the same feeRate that was active during the deposit is stored in a mapping along with the vault id. This feeRate would be used until the call option has expired. This will allow the vault owner to withdraw their deposit if they don't agree to the new rate.

[outdoteth]

owner can change fee at any time; #47
owner can set fee greater than 100%: #48