Direct theft of any user NFTs, whether at-rest or in-motion, other than unclaimed royalties
Description
Brief/Intro
The ThunderExchange contract contains a critical vulnerability that could allow an attacker to manipulate buy/sell order and potentially gain unauthorized ownership of NFTs.
This vulnerability stems from insufficient checks during order updates and cancellations, particularly when changing an order from buy to sell.
Vulnerability Details
The vulnerability exists in the interaction between the update_order and cancel_order functions. Here's a step-by-step breakdown:
An attacker can place a buy order at a very low price for an NFT they don't own. All they need to do is to make sure they have pool balance greater than order price:
Attacker can then update this buy order to a sell order:
Note that changing sides is allowed in _validate_maker_order_input. Also, crucially, there is no check if the attacker indeed owns the NFT if order side is changed.
The attacker can then cancel this "sell" order:
Note that on cancellation, the NFT asset is transferred back to the maker, only this time, the maker was never the original owner of the NFT.
The vulnerability arises because:
There's no check to prevent changing order sides during updates.
There's no verification of NFT ownership when updating to a sell order.
The cancellation process trusts the strategy contract to return the correct order information without additional verification.
Impact Details
In the worst-case scenario, an attacker could:
Create buy orders for valuable NFTs they don't own.
Update these to sell orders without owning the NFTs.
Cancel these "sell" orders, potentially receiving NFTs they never owned.
This could lead to unauthorized transfer of high-value NFTs, significant financial losses for legitimate NFT owners, and a complete breakdown of trust in the marketplace.
Note: the commented test code in ThunderSDK contains old contracts that are no longer in use. All tests are commented as the function signatures in the tests don't match with the code in scope
New ABIs need to be generated for all contracts. I have created this setup using latest contracts.
fn place_order(order_input: MakerOrderInput) {
_validate_maker_order_input(order_input);
// ... (no NFT transfer for buy orders)
strategy.place_order(order);
}
fn update_order(order_input: MakerOrderInput) {
_validate_maker_order_input(order_input);
// ... (no NFT ownership check)
match order.side {
Side::Buy => {
// ... (check pool balance)
},
Side::Sell => {}, // No checks for sell orders
}
strategy.update_order(order);
}
fn cancel_order(strategy: ContractId, nonce: u64, side: Side) {
// ...
let order = strategy_caller.get_maker_order_of_user(caller, nonce, side);
match side {
Side::Sell => {
if (order.is_some()) {
// ... (cancel order in strategy)
transfer(
Identity::Address(unwrapped_order.maker),
AssetId::new(unwrapped_order.collection, unwrapped_order.token_id),
unwrapped_order.amount
);
}
},
}
}
