Cally contest - horsefacts's results

Judge has assessed an item in Issue #294 as Medium risk. The relevant finding follows:

Owner can frontrun exercise to increase fees A malicious owner account can observe and frontrun calls to exercise and extract 100% of the strike price as a protocol fee.

Scenario:

A malicious owner observes a call to exercise in the mempool. The owner frontruns exercise and calls setFee to set feeRate to 100% The full strike price is paid as a protocol fee, and 0 ETH are credited to the vault beneficiary. Recommendation: Ensure the contract owner is a timelock proxy with a waiting period for parameter changes. Emit an event on changes to feeRate (See N-01 below).

Judge has assessed an item in Issue #294 as Medium risk. The relevant finding follows:

Beneficiary is credited additional ETH above premium The Cally#buyOption function ensures that the caller sends an ETH amount equal to or greater than the calculated premium:

buyOption#L224

   require(msg.value >= premium, "Incorrect ETH amount sent");

It then credits the beneficiary with an amount equal to msg.value:

buyOption#L250

    ethBalance[beneficiary] += msg.value;

If the caller of buyOption sends excess ETH above the premium amount, this additional amount is credited to the beneficiary.

Recommendation: If this is intentional, clearly document this behavior for end users. If not, consider requiring an exact premium amount rather than accepting additional ETH.

Lines of code

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

Vulnerability details

Rebasing tokens lock excess balance in contract

If a vault is created with a rebasing ERC20 as its token, additional balance accrued through rebases while the token is owned by the vault may be locked in the contract.

If the rebasing token balance decreases while owned by the vault, this is the same scenario as a fee on transfer token that blocks withdrawals and exercise. However, if the balance increases (for example, a positive rebasing token like stETH), the original balance may be withdrawn, but the accrued amount will be locked.

Scenario:

Setup:

1. Alice calls createVault with a rebasing token address and 1000 tokenIdOrAmount.
2. The vault will be created and stored with a tokenIdOrAmount equal to 1000.
3. Cally.sol#createVault transfers in 1000 tokens.
4. The token contract rebases balances, and the vault accrues an additional 10 tokens.

Withdrawal: 4a. If Alice initiates and executes a withdrawal, Cally.sol#withdraw will transfer out vault.tokenIdOrAmount, or 1000 tokens. The additional 10 accrued tokens will be locked in the contract.

Exercise: 4b. If Bob buys and exercises an option, Cally.sol#exercise will transfer out vault.tokenIdOrAmount, or 1000 tokens. The additional 10 accrued tokens will be locked in the contract.

Test cases

In the following test cases, the rebasing token is a MockERC20 that simulates a 1% positive rebase when rebase(address) is called.

Exercise.t.sol:

    function testRebasingLocksExcessBalance() public {
        // arrange
        vm.prank(babe);
        vaultId = c.createVault(tokenAmount, address(rebasing), 1, 1, 1, 0, Cally.TokenType.ERC20);
        vault = c.vaults(vaultId);
        optionId = c.buyOption{value: premium}(vaultId);
        uint256 balanceBefore = fees.balanceOf(address(this));
        rebasing.rebase(address(c));

        // act
        c.exercise{value: strike}(optionId);
        uint256 change = rebasing.balanceOf(address(this)) - balanceBefore;

        // assert
        assertEq(change, tokenAmount, "Should have transferred REBASE to exerciser");
        assertEq(rebasing.balanceOf(address(c)), 0, "Should have transferred REBASE from Cally");
    }

Withdraw.t.sol:

    function testWithdrawLocksExcessBalanceForRebasingToken() public {
        // arrange
        vaultId = c.createVault(tokenAmount, address(rebasing), 1, 1, 1, 0, Cally.TokenType.ERC20);
        c.initiateWithdraw(vaultId);
        skip(1);
        rebasing.rebase(address(c));
        uint256 balanceBefore = rebasing.balanceOf(address(this));

        // act
        c.withdraw(vaultId);
        uint256 balanceAfter = rebasing.balanceOf(address(this));

        // assert
        assertEq(balanceAfter - balanceBefore, tokenAmount, "Should have transferred FEES to owner");
        assertEq(rebasing.balanceOf(address(c)), 0, "Should have transferred FEES from Cally");
    }

Recommendation

Supporting every weird ERC20 is difficult, and rebasing tokens are harder to detect than fee-on-transfer tokens. If feasible, use a token allowlist, or at least limit supported tokens in your UI and document the risks of nonstandard tokens for end users.

Lines of code

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

Vulnerability details

Fee on transfer tokens block exercises and withdrawals

If a vault is created with a fee-on-transfer ERC20 as its token, the underlying asset may be locked in the contract.

Scenario:

Setup:

1. Alice calls createVault with a fee-on-transfer token address and 1000 tokenIdOrAmount. The token takes a 1% fee on transfers.
2. The vault will be created and stored with a tokenIdOrAmount equal to 1000.
3. Cally.sol#createVault transfers in 1000 tokens, but receives 990 tokens after fees.

Withdrawal: 4a. If Alice initiates and executes a withdrawal, Cally.sol#withdraw will attempt to transfer out vault.tokenIdOrAmount tokens. Since the stored value is 1000, but the contract balance is only 990, this transfer will revert.

Exercise: 4b. If Bob buys and exercises an option, Cally.sol#exercise will attempt to transfer out vault.tokenIdOrAmount tokens. Since the stored value is 1000, but the contract balance is only 990, this transfer will revert.

Test cases

In the following test cases, the fees token is a MockERC20 that simulates a 1% fee on transfer.

Exercise.t.sol:

    function testExerciseRevertsForFeeOnTransfer() public {
        // arrange
        vm.prank(babe);
        vaultId = c.createVault(tokenAmount, address(fees), 1, 1, 1, 0, Cally.TokenType.ERC20);
        vault = c.vaults(vaultId);
        optionId = c.buyOption{value: premium}(vaultId);
        uint256 balanceBefore = fees.balanceOf(address(this));

        // act
        c.exercise{value: strike}(optionId);
        uint256 change = fees.balanceOf(address(this)) - balanceBefore;

        // assert
        assertEq(change, tokenAmount, "Should have transferred FEES to exerciser");
        assertEq(fees.balanceOf(address(c)), 0, "Should have transferred FEES from Cally");
    }

Withdraw.t.sol:

    function testWithdrawRevertsForFeeOnTransfer() public {
        // arrange
        vaultId = c.createVault(tokenAmount, address(fees), 1, 1, 1, 0, Cally.TokenType.ERC20);
        c.initiateWithdraw(vaultId);
        skip(1);
        uint256 balanceBefore = fees.balanceOf(address(this));

        // act
        c.withdraw(vaultId);
        uint256 balanceAfter = fees.balanceOf(address(this));

        // assert
        assertEq(balanceAfter - balanceBefore, tokenAmount, "Should have transferred FEES to owner");
        assertEq(fees.balanceOf(address(c)), 0, "Should have transferred FEES from Cally");
    }

Recommendation

Supporting every weird ERC20 is difficult. If feasible, use a token allowlist, or at least limit supported tokens in your UI and document the risks of nonstandard tokens for end users.

Consider checking the balance before and after ERC20 transfers and reverting if the contract receives less than the expected amount. However, note that this is not a perfect solution either, since some tokens (like Tether and USDC) have transfer fees that are currently zero but may be enabled in the future.

Low

[L-01] Owner can frontrun exercise to increase fees

A malicious owner account can observe and frontrun calls to exercise and extract 100% of the strike price as a protocol fee.

Scenario:

1. A malicious owner observes a call to exercise in the mempool.
2. The owner frontruns exercise and calls setFee to set feeRate to 100%
3. The full strike price is paid as a protocol fee, and 0 ETH are credited to the vault beneficiary.

Recommendation: Ensure the contract owner is a timelock proxy with a waiting period for parameter changes. Emit an event on changes to feeRate (See N-01 below).

[L-02] Beneficiary is credited additional ETH above premium

The Cally#buyOption function ensures that the caller sends an ETH amount equal to or greater than the calculated premium:

buyOption#L224

       require(msg.value >= premium, "Incorrect ETH amount sent");

It then credits the beneficiary with an amount equal to msg.value:

buyOption#L250

        ethBalance[beneficiary] += msg.value;

If the caller of buyOption sends excess ETH above the premium amount, this additional amount is credited to the beneficiary.

Recommendation: If this is intentional, clearly document this behavior for end users. If not, consider requiring an exact premium amount rather than accepting additional ETH.

QA

[N-01] Emit events for permissioned parameter changes

The permissioned function setFee updates the feeRate parameter, but does not emit an event. Consider emitting a SetFee event that logs the previous and new feeRate values. This enables you to monitor off chain for suspicious activity, and allows end users to observe and trust permissioned changes to this parameter.

Cally.sol#setFee

    /// @notice Sets the fee that is applied on exercise
    /// @param feeRate_ The new fee rate: fee = 1% = (1 / 100) * 1e18
    function setFee(uint256 feeRate_) external onlyOwner {
        feeRate = feeRate_;
    }

Gas

Set isExercised directly in exercise

Setting the single attribute isExercised directly in exercise is cheaper than storing the full vault struct.

Current:

Cally.sol#277

        // mark the vault as exercised
        vault.isExercised = true;
        _vaults[vaultId] = vault;

Gas usage: 49631 avg, 65473 median, 87933 max

Optimized:

        // mark the vault as exercised
        _vaults[vaultId].isExercised = true;

Gas usage: 48858 avg, 64413 median, 86873 max