Contract fails to deliver promised returns, but doesn't lose value
Block stuffing
Description
Brief/Intro
the function update_order meant to allow users with current active bid to update their order important input, however there is critical check that prevent users to update their bid and they all forced to cancel their bid, this will lead to lose of gas only for buy order users. this happens because the update_order calls the update_order in the fixed strategy which in return it calls the _update_buy_order which checks if the payment asset in unchanged, the payment asset is same asset that set in whitelist when assetManger call add asset or remove it by calling remove asset.
same thing can be applied to sell order users which is more critical situation compared to buy order users, we can take the steps below that can happen when the paymentAsset removed and new asset added:
Alice create sell order(listing NFT) by calling place_order with paymentAsset == USDT.
10 users create buy order to bid on alice NFT, and their payment asset == USDT.
for some reason, USDT removed from whitelisted address by calling the assetManger.sw#remove_asset and ETH added as paymentAsset by calling add_asset function.
because of the check(order.unwrap().payment_asset == updated_order.payment_asset) which check the payment asset of old order and updated one in _validate_updated_order which invoked by _update_buy and _update_sell alice and all other 10 users with bid order can not update their bid order payment asset to eth, same true for alice.
this way alice and other users are forced to remove their orders(cancle it) and re create same order again and add new bid on the alice NFT which lead to lose of gas too.
Vulnerability Details
the function update_order implemented as below:
#[storage(read), payable]
fn update_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 balance
let pool_balance = _get_pool_balance(order.maker, order.payment_asset);
require(order.price <= pool_balance, ThunderExchangeErrors::AmountHigherThanPoolBalance);
},
Side::Sell => {}, // if order is selling nft then nothing to check for
}
strategy.update_order(order);
log(OrderUpdated {
order
});
}
which invoke the update_order function for both buy and sell order:
/// Updates the existing MakerOrder of the user
/// Only callable by Thunder Exchange contract
#[storage(read, write)]
fn update_order(order: MakerOrder) {
// only_exchange();
match order.side {
Side::Buy => {
_update_buy_order(order)
},
Side::Sell => {
_update_sell_order(order)
}
}
}
both _update_buy_order and _update_sell_order invoke the _validate_updated_order function as shown below
/// Updates buy MakerOrder if the nonce is in the right range
#[storage(read, write)]
fn _update_buy_order(order: MakerOrder) {
let nonce = _user_buy_order_nonce(order.maker); // nonce should not be changed for update purpose same to maker address
if ((order.nonce <= nonce)) {
// Update buy order
let buy_order = _buy_order(order.maker, order.nonce); // get the old order
_validate_updated_order(buy_order, order);
storage.buy_order.insert((order.maker, order.nonce), Option::Some(order));
} else {
revert(114);
}
}
/// Updates sell MakerOrder if the nonce is in the right range
#[storage(read, write)]
fn _update_sell_order(order: MakerOrder) {
let nonce = _user_sell_order_nonce(order.maker);
let min_nonce = _user_min_sell_order_nonce(order.maker);
if ((min_nonce < order.nonce) && (order.nonce <= nonce)) {
// Update sell order
let sell_order = _sell_order(order.maker, order.nonce);
_validate_updated_order(sell_order, order);
storage.sell_order.insert((order.maker, order.nonce), Option::Some(order));
} else {
revert(115);
}
}
#[storage(read)]
fn _validate_updated_order(order: Option<MakerOrder>, updated_order: MakerOrder) {
require(
(order.unwrap().maker == updated_order.maker) &&
(order.unwrap().collection == updated_order.collection) &&
(order.unwrap().token_id == updated_order.token_id) &&
(order.unwrap().payment_asset == updated_order.payment_asset) && //@audit payment asset should be allowed to be changed
_is_valid_order(order),
StrategyFixedPriceErrors::OrderMismatchedToUpdate
);
}
this way its impossible to update the current bids and listing NFT in this situation and lead to cancel all orders and re execute it again.
Impact Details
users can't update their orders when paymentAsset removed and another one added.
Recommend
there is no reason to check for updated order payment if its same as old one since in the flow we check the updated order if its payment asset is whitelisted or not.
Proof of concept
Proof of Concept
run this poc in contract-v1/tests/src/main.sw with following these details below:
details should follow to execute the test in sway
for anyone want to create POC using sway itself, i created a simple template do so, all thanks to @theschnilch who helped a lot to make this test file valid.
to create POC or doing some tests in sway do the follow:
create forc.toml in contracts-v1 and add the below in the forc.toml:
and after that you can add the code below in your 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,
};
use libraries::{
msg_sender_address::*,
constants::*,
order_types::*,
ownable::*,
};
use std::{
block::timestamp,
auth::*,
call_frames::*,
context::*,
contract_id::ContractId,
logging::log,
revert::require,
storage::storage_map::*,
asset::*,
};
#[test(should_revert)]
fn test_attack() {
initialize_functions();
}
fn initialize_functions() {
//initialize all contracts
let thunder_exch = abi(ThunderExchange, thunder_exchange::CONTRACT_ID);
thunder_exch.initialize();
let asset_mngr = abi(AssetManager, asset_manager::CONTRACT_ID);
asset_mngr.initialize();
// required for initialize below contracts
let exchange_contract_id = ContractId::from(thunder_exchange::CONTRACT_ID);
let asset_manger_contract_id = ContractId::from(asset_manager::CONTRACT_ID);
let fixed_strategy = abi(ExecutionStrategy, strategy_fixed_price_sale::CONTRACT_ID);
fixed_strategy.initialize(exchange_contract_id);
let execution_manager = abi(ExecutionManager, execution_manager::CONTRACT_ID);
execution_manager.initialize();
let royalty_manager = abi(RoyaltyManager, royalty_manager::CONTRACT_ID);
royalty_manager.initialize();
//get contract IDs
let execution_manger_contract = ContractId::from(execution_manager::CONTRACT_ID);
let royalty_manger_contract = ContractId::from(royalty_manager::CONTRACT_ID);
let pool_contract = ContractId::from(pool::CONTRACT_ID);
let stratgey_fixed = ContractId::from(strategy_fixed_price_sale::CONTRACT_ID);
let asset_to_add = AssetId::base();
asset_mngr.add_asset(asset_to_add);
let asset_id = AssetId::bits(asset_to_add);
let pool = abi(Pool, pool::CONTRACT_ID);
pool.initialize(exchange_contract_id, asset_manger_contract_id);
let caller = Address::from(0xf8f8b6283d7fa5b672b530cbb84fcccb4ff8dc40f8176ef4544ddb1f1952ad07);
pool.update_balance(asset_to_add, 100u64);
//set functions
execution_manager.add_strategy(stratgey_fixed);
thunder_exch.set_asset_manager(asset_manger_contract_id);
thunder_exch.set_pool(pool_contract);
thunder_exch.set_execution_manager(execution_manger_contract);
// thunder_exch.set_protocol_fee_recipient(Identity::Address::from(0xf9b8b6283d7fa5b672b530cbb84fcccb4ff8dc40f8176ef4544ddb1f1952ad07)); // diff from maker
thunder_exch.set_royalty_manager(royalty_manger_contract);
//----------------------execute attack --------------------------------//
let caller = Address::from(0xf9b8b6283d7fa5b672b530cbb84fcccb4ff8dc40f8176ef4544ddb1f1952ad07);
let extra_parameter = ExtraParams {
extra_address_param: Address::zero(),
extra_contract_param: ContractId::zero(),
extra_u64_param: 0u64
};
let maker_order = MakerOrderInput {
side: Side::Buy,
maker:caller,
collection: ContractId::from(0x7777777777777777777777777777777777777777777777777777777777777777),
token_id: SubId::from(0x7777777777777777777777777777777777777777777777777777777777777778),
price: 10u64,
amount: 1u64,
nonce: 1u64,
strategy:stratgey_fixed,
payment_asset: asset_to_add,
expiration_range: 1000u64,
extra_params: extra_parameter,
};
thunder_exch.place_order(maker_order);
//now remove the whitelist token and add another one
asset_mngr.remove_asset(0);
let asset_to_add1 = AssetId::from(0x7777_7777_7777_7777_7777_7777_7777_7777_7777_7777_7777_7777_7777_7777_7777_7777);
asset_mngr.add_asset(asset_to_add1);
//call update with new paymentAsset
pool.update_balance(asset_to_add1, 100u64);
let maker_order1 = MakerOrderInput {
side: Side::Buy,
maker:caller,
collection: ContractId::from(0x7777777777777777777777777777777777777777777777777777777777777777),
token_id: SubId::from(0x7777777777777777777777777777777777777777777777777777777777777778),
price: 10u64,
amount: 1u64,
nonce: 1u64,
strategy:stratgey_fixed,
payment_asset: asset_to_add1,
expiration_range: 1000u64,
extra_params: extra_parameter,
};
thunder_exch.update_order(maker_order1);
}
NOTES TO TAKE IN MIND BEFORE RUNNING THE POC:
thunderNFT does not support sway test so we create one, and while sway is under development, there is some change we made to the in scope contracts to execute the POC:
first we set the EXTRAPARAMS to public by adding pub keyword to it: this because sway test files does not allow using internal function or struct.
in _validate_maker_order_input we commented the line below, because sway itself does not allow using msg.sender or control it so its possible to execute call to place_order with maker equal to msg.sender.
// require(input.maker == get_msg_sender_address_or_panic(), ThunderExchangeErrors::CallerMustBeMaker); // maker of order should be the caller
we added a simple function to update the user balance in the pool:
this is because sway test files does not allow calling payable function with asset id and coins at all, so we need to find way to update the user pool balance to simulate user depositing tokens:
