Direct theft of any user funds, whether at-rest or in-motion, other than unclaimed yield
Description
Brief/Intro
The update_order function lacks proper validation for sell orders, allowing attackers to forge asset amounts without transferring the actual assets. This critical vulnerability in the order update mechanism could be exploited in production to create sell orders with artificially inflated asset amounts. Subsequently, attackers could cancel these fraudulent orders and withdraw assets they never actually deposited, potentially leading to significant theft of assets from the exchange or other users' funds, undermining the entire trading system's integrity and security.
Vulnerability Details
To explain this vulnerability, let's first look at the code for the place_order function. It is a payable function that checks msg_asset_id() and msg_amount() when a user creates a sell order.
#[storage(read), payable]fnplace_order(order_input:MakerOrderInput) {_validate_maker_order_input(order_input);let strategy =abi(ExecutionStrategy, order_input.strategy.bits());let order =MakerOrder::new(order_input);match order.side {Side::Buy=> {// Buy MakerOrder (e.g. make offer)// Checks if user has enough bid balancelet pool_balance =_get_pool_balance(order.maker, order.payment_asset);require(order.price <= pool_balance, ThunderExchangeErrors::AmountHigherThanPoolBalance); },Side::Sell=> {// Sell MakerOrder (e.g. listing)// Checks if assetId and amount mathces with the order require(msg_asset_id() == AssetId::new(order.collection, order.token_id), ThunderExchangeErrors::AssetIdNotMatched);
require(msg_amount() == order_input.amount, ThunderExchangeErrors::AmountNotMatched); // <---- proper checks here
}, } strategy.place_order(order);log(OrderPlaced { order }); }
But in update_order, there is no such check, which allow user can update the amount of selling asset.
fnupdate_order(order_input:MakerOrderInput) {_validate_maker_order_input(order_input);let strategy =abi(ExecutionStrategy, order_input.strategy.bits());let order =MakerOrder::new(order_input);match order.side {Side::Buy=> {// Checks if user has enough bid balancelet pool_balance =_get_pool_balance(order.maker, order.payment_asset);require(order.price <= pool_balance, ThunderExchangeErrors::AmountHigherThanPoolBalance); },Side::Sell=> {}, // <---- No checks for msg_assetid() and msg_amount() } strategy.update_order(order);log(OrderUpdated { order }); }
At last, in cancel_order, the contract allows user withdraw his assets base on the amount claimed in order.
#[storage(read)]fncancel_order( strategy:ContractId, nonce:u64, side:Side ) {let caller =get_msg_sender_address_or_panic();let execution_manager_addr = storage.execution_manager.read().unwrap().bits();let execution_manager =abi(ExecutionManager, execution_manager_addr);require(strategy != ZERO_CONTRACT_ID, ThunderExchangeErrors::StrategyMustBeNonZeroContract);require(execution_manager.is_strategy_whitelisted(strategy), ThunderExchangeErrors::StrategyNotWhitelisted);let strategy_caller =abi(ExecutionStrategy, strategy.bits());let order = strategy_caller.get_maker_order_of_user(caller, nonce, side);match side {Side::Buy=> {// Cancels buy MakerOrder (e.g. offer) strategy_caller.cancel_order(caller, nonce, side); },Side::Sell=> {// Cancel sell MakerOrder (e.g. listing)if (order.is_some()) {// If order is valid, then transfers the asset back to the userlet unwrapped_order = order.unwrap(); strategy_caller.cancel_order(caller, nonce, side);transfer( Identity::Address(unwrapped_order.maker), AssetId::new(unwrapped_order.collection, unwrapped_order.token_id), unwrapped_order.amount // <---- This value could be forged by update_order ); } }, }log(OrderCanceled { user: caller, strategy, side, nonce, }); }
Such improper input validation case could lead to direct asset theft, I'll show the detail below.
Fix advice
Check msg_asset_id() and msg_amount() in update_order when updating sell order.
Distingulish ERC-721 and multi native asset, choose the correct amount in ExecutionResult:
First, this is a critical level bug which allow attacker steal assets selling on the platfrom. I believe the underlying issue here is that the smart contract only considered scenarios involving ERC-721 NFT trading, without taking into account situations involving ERC-1155 NFTs or other native assets.
As per defined in Multi Native Asset Example, multi native asset model is equivalent to ERC-1155 Standard use in Ethereum. For single native asset (ERC-721), there is only one asset for each AssetId, but for multi native asset (ERC-11555), it allows multiple asset for each AssetId.
The following is a detailed attack scenario:
Victim UserA wants to sell 1000 of his NFT/native assets called ERC1155_NFT on platfrom, he creates a sell order by calling place_order, then 1000 ERC1155_NFT sent to the thunder exchange contract.
Attacker UserB wants to steal those 1000 ERC1155_NFT, he creates a sell order by calling place_order with 1 ERC1155_NFT, then his 1 ERC1155_NFT sent to the thunder exchange contract.
Attacker UserB calls update_order to increase the amount of his sell order to 1000 ERC1155_NFT, becase update_order does not require a deposit and check msg_amount(), this operation success.
Attacker UserB calls cancel_order to withdraw 1000 ERC1155_NFT from thunder exchange contract, because the transfer amount is based on unwrapped_order.amount. Now UserB has 1000 ERC1155_NFT and only 1 ERC1155_NFT left on exchange contract.
