Submitted on Thu Aug 22 2024 21:39:01 GMT-0400 (Atlantic Standard Time) by @jecikpo for IOP | ThunderNFT
Report ID: #34736
Report type: Smart Contract
Report severity: Critical
Target: https://github.com/ThunderFuel/smart-contracts/tree/main/contracts-v1/libraries
Impacts:
- Permanent freezing of NFTs
When a Sell order is executed the amount of assets sold to the taker is always 1. Once an order is executed it is deleted. If there were more than 1 asset amount on that order they are locked in the exchange permanently
The Thunder exchange supports also ERC1155 tokens which could be provided during placement of the Sell order by calling the place_order()
method.
The contract validate if the order input data matches with the assets supplied to the contract by the seller using the following code:
require(msg_asset_id() == AssetId::new(order.collection, order.token_id), ThunderExchangeErrors::AssetIdNotMatched);
require(msg_amount() == order_input.amount, ThunderExchangeErrors::AmountNotMatched);
When a Sell order is executed by calling the execute_order()
method the amount
to be sold is hardcoded to 1
in the ExecutionResult
function s1()
as per the following snippet:
pub fn s1(maker_order: MakerOrder, taker_order: TakerOrder) -> ExecutionResult {
ExecutionResult {
is_executable: (
[ . . . ]
),
collection: taker_order.collection,
token_id: taker_order.token_id,
amount: 1,
payment_asset: maker_order.payment_asset,
}
}
This means that only single asset is sold. Later on during the order processing, the order gets deleted in the _execute_order()
function.
Once an order is deleted:
- it cannot be further executed, hence no more of the remaining assets can be sold.
- it cannot be cancelled by the maker, hence the assets cannot be withdrawn back to the original owner.
As the ERC1155 should be supported, the impact of this bug is Critical because it leads to the seller's assets being permanently stuck on the contract in case they want to sell more than one (which is usually the case with ERC1155).
The amount
to be sold needs to be dynamic and taken from the TakerOrder
input data (which needs to be modified). Also the amount inside the order must be adjusted. Solution to this issue needs to be added into multiple contracts, hence needs to be carefully though of.
The problematic line in the libraries: https://github.com/ThunderFuel/smart-contracts/blob/260c9859e2cd28c188e8f6283469bcf57c9347de/contracts-v1/libraries/src/execution_result.sw#L31
Can be found here: https://gist.github.com/c8a90b54808923309280d9624ae01401.git