Setting a high feeRate can block `exercise` or cause negative flow of funds

[code423n4]

Lines of code

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

Vulnerability details

Impact

When an admin intentionally or unintentionally sets a feeRate greater than 1e18 (100%),

• The exercise function can fail with arithmetic operation underflow at line 289
• In the case, when beneficiary is connected to multiple vaults, the exercise function will succeed, but would cause negative flow of funds from beneficiary, and beneficiary will get lesser amout during harvest.

line 289,

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

All option buyers, who want to exercise are effected.

Proof of Concept

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

Tools Used

Manual review & test

Recommended Mitigation Steps

Define a global constant, say

uint256 public constant MAX_FEE_RATE = 5 * 1e17; // 5%

And add a require statement in setFee() to check against the max value that can be set

require(feeRate_ <= MAX_FEE_RATE, "Protocol fees very high");

[outdoteth]

owner can set fee greater than 100%: https://github.com/code-423n4/2022-05-cally-findings/issues/48