Direct theft of any user funds, whether at-rest or in-motion, other than unclaimed yield
Description
Thunder Exchange
Incorrect Token Sale Amount
Description
An issue has been identified that allows a malicious actor to sell only one token, even if the Buy Order specifies a greater quantity. This vulnerability effectively bypasses the intended order amount.
Root Cause
As discussed in our Discord exchange, the Order.amount field was introduced to accommodate ERC1155-style tokens:
Hi! Yes, amount is added in case of Erc1155 style token standard
This clearly states that the Order.amount can be greater than 1. However, when executing an order, the ExecutionResult has a hardcoded amount of 1:
As a result, the specified amount in the Order is effectively ignored.
Impact
This issue has two significant impacts:
Buy Order
For a Maker Order of type Buy, an attacker can sell a single token instead of the specified quantity, receiving full payment and effectively defrauding the buyer.
Sell Order
For a Maker Order of type Sell, an innocent buyer may pay the full price but only receive a single token, with the remainder of the tokens being locked away.
Proposed fix
To resolve this issue, the maker_order.amount should be included when crafting the ExecutionResult.
Proof of concept
Proof of Concept
Due to the relatively new nature of the Sway language and the limited availability of reliable testing tools, the proof of concept (PoC) is complex.
Below is a pseudo-PoC followed by a detailed actual PoC.
Pseudo POC
An innocent user creates a Buy Order for an asset, specifying an amount greater than one.
A malicious actor executes the Buy Order but only sells one token.
// innocent user exchange.place_order(order);// attacker exchange.execute_order{ asset_id: AssetId::new(order.collection, order.token_id).bits(), coins: 1 }(order); // attackers sells only 1 token although the users asked for 100
Actual POC
There are some steps to follow:
To create an actual PoC, some modifications to the protocol are necessary.
The protocol currently only allows addresses to interact, which is generally fine. However, when transferring coins in the Fuel ecosystem, a Variable Output needs to be added to the transaction — this isn't supported by the current Sway testing tools.
Since this vulnerability is unrelated to the nature of who interacts with the protocol, adjustments will be made to allow contracts to interact, enabling the PoC. These changes are strictly for testing purposes and do not affect the core issue.
Changes to the protocol for the POC
Add the following changes to the thunder_exchange contract:
// line 141 change from:let caller =get_msg_sender_address_or_panic();// change tolet caller =Address::from(get_msg_sender_contract_or_panic().bits()); // changed for POC/* ------------------------------------------------------------------------------------------------------ */// line 162 change from:Identity::Address(unwrapped_order.maker),// change toIdentity::ContractId(ContractId::from(unwrapped_order.maker.bits())), // changed for POC/* ------------------------------------------------------------------------------------------------------ */// line 324 change from:require(input.maker ==get_msg_sender_address_or_panic(), ThunderExchangeErrors::CallerMustBeMaker);// change to require(input.maker == Address::from(get_msg_sender_contract_or_panic().bits()), ThunderExchangeErrors::CallerMustBeMaker); // changed for POC
/* ------------------------------------------------------------------------------------------------------ */// line 352 change from:require(taker_order.taker ==get_msg_sender_address_or_panic(), ThunderExchangeErrors::CallerMustBeMaker);// change to require(taker_order.taker == Address::from(get_msg_sender_contract_or_panic().bits()), ThunderExchangeErrors::CallerMustBeMaker); // changed for POC
/* ------------------------------------------------------------------------------------------------------ */// line 388 change from:Identity::Address(order.taker),// change toIdentity::ContractId(ContractId::from(order.taker.bits())), // changed for POC/* ------------------------------------------------------------------------------------------------------ */// line 408 change from:Identity::Address(order.maker),// change toIdentity::ContractId(ContractId::from(order.maker.bits())), // changed for POC/* ------------------------------------------------------------------------------------------------------ */// line 454 change from:transfer(Identity::Address(to), payment_asset, final_seller_amount);// change to transfer(Identity::ContractId(ContractId::from(to.bits())), payment_asset, final_seller_amount); // changed for POC
/* ------------------------------------------------------------------------------------------------------ */// line 474 change from:Identity::Address(from),// change toIdentity::ContractId(ContractId::from(from.bits())), // changed for POC/* ------------------------------------------------------------------------------------------------------ */// line 508 change from:transfer(Identity::Address(to), payment_asset, final_seller_amount);// change to transfer(Identity::ContractId(ContractId::from(to.bits())), payment_asset, final_seller_amount); // changed for POC
/* ------------------------------------------------------------------------------------------------------ */// line 521 change from: pool.balance_of(Identity::Address(account), asset)// change to pool.balance_of(Identity::ContractId(ContractId::from(account.bits())), asset) // changed for POC
In addition, To make an MakerOrderInput Struct, we need to make the ExtraParams Struct public, so in the order_types.sw file, lets add a pub in line 37.
The tests files.
Create forc.toml in contracts-v1 and add the below in the forc.toml: