code-423n4 / 2022-05-cally-findings

2 stars 0 forks source link

Lack maximum value of `feeRate` #210

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

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#L282-L289

Vulnerability details

Impact

setFee allow the owner to see feeRate to any value, this lead to 2 problems.

  1. If feeRate > 1e18, user will not be able to exercise options as L289 would revert due to underflow.
  2. Owner can set feeRate to 100% to rug user

Proof of Concept

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

    function setFee(uint256 feeRate_) external onlyOwner {
        feeRate = feeRate_;
    }

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

        uint256 fee = 0;
        if (feeRate > 0) {
            fee = (msg.value * feeRate) / 1e18;
            protocolUnclaimedFees += fee;
        }

        // increment vault beneficiary's ETH balance
        ethBalance[getVaultBeneficiary(vaultId)] += msg.value - fee;

Recommended Mitigation Steps

Add a check to make sure feeRate is below some constant value e.g. 10%

outdoteth commented 2 years ago

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