IOP _ ThunderNFT 34455 - [Smart Contract - Low] Double Token Vulnerability leads to drain funds

Submitted on Tue Aug 13 2024 04:28:52 GMT-0400 (Atlantic Standard Time) by @bugtester for IOP | ThunderNFT

Report ID: #34455

Report type: Smart Contract

Report severity: Low

Target: https://github.com/ThunderFuel/smart-contracts/tree/main/contracts-v1/pool

Impacts:

• Direct theft of any user funds, whether at-rest or in-motion, other than unclaimed yield

Description

Brief/Intro

The vulnerability resided within the smart contract's _transfer function, which is responsible for handling balance update. This function unfortunately relied on cached balance values for both the sender and recipient of the tokens. This mechanism created a loophole , leading to a double token vulnerability.

https://github.com/ThunderFuel/smart-contracts/blob/260c9859e2cd28c188e8f6283469bcf57c9347de/contracts-v1/pool/src/main.sw#L237C1-L257C2

fn _transfer(from: Identity, to: Identity, asset: AssetId, amount: u64) {
    require(
        to != ZERO_IDENTITY_ADDRESS &&
        to != ZERO_IDENTITY_CONTRACT,
        PoolErrors::IdentityMustBeNonZero
    );

    let from_balance = _balance_of(from, asset);
    let to_balance = _balance_of(to, asset);
    require(from_balance >= amount, PoolErrors::AmountHigherThanBalance);

    storage.balance_of.insert((from, asset), from_balance - amount);
    storage.balance_of.insert((to, asset), to_balance + amount);

    log(Transfer {
        from,
        to,
        asset,
        amount,
    });
}

Impact

This vulnerability can lead to an incorrect cache balance state, potentially allowing attacker to duplicate tokens and exploit the contract for drain funds

References

few months back Minerercx was exploit with same issue - https://techfund.jp/en/media/Miner-Vulnerability-Exploited

Proof of concept

Proof of Concept

Steps to Reproduce:

Call the _transfer function with the same from and to identity. Observe that the balance is incremented twice, leading to token duplication. due to cache balance

example

The transferfunction was triggered with the from and to addresses being identical, signifying a self-transfer.

Before the actual transfer took place, the function retrieved the cached balance values for both the sender and recipient (which was the same address in this case).

let from_balance = _balance_of(from, asset);
let to_balance = _balance_of(to, asset);

The function then subtracted the transfer amount from the cached sender balance and added the same amount to the cached recipient balance.

Since both cached values pointed to the same address, this operation essentially doubled the user's balance, leading to the exploitation

fix

fn _transfer(from: Identity, to: Identity, asset: AssetId, amount: u64) {
    require(
        to != ZERO_IDENTITY_ADDRESS &&
        to != ZERO_IDENTITY_CONTRACT,
        PoolErrors::IdentityMustBeNonZero
    );
    
    // Check if from and to identities are the same
    if from == to {
        // If the same, ensure the balance is sufficient and log without updating
        let balance = _balance_of(from, asset);
        require(balance >= amount, PoolErrors::AmountHigherThanBalance);
        log(Transfer {
            from,
            to,
            asset,
            amount,
        });
        return;
    }

    let from_balance = _balance_of(from, asset);
    let to_balance = _balance_of(to, asset);
    require(from_balance >= amount, PoolErrors::AmountHigherThanBalance);

    storage.balance_of.insert((from, asset), from_balance - amount);
    storage.balance_of.insert((to, asset), to_balance + amount);

    log(Transfer {
        from,
        to,
        asset,
        amount,
    });
}