code423n4 commented 2 years ago

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:

Alice calls createVault with a rebasing token address and 1000 tokenIdOrAmount.
The vault will be created and stored with a tokenIdOrAmount equal to 1000.
Cally.sol#createVault transfers in 1000 tokens.
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.

outdoteth commented 2 years ago

we have no intention to support rebase tokens