Fee on transfer tokens block exercises and withdrawals

[code423n4]

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.

[outdoteth]

reference issue: https://github.com/code-423n4/2022-05-cally-findings/issues/39