Starknet Auction
First Flight #26
Beginner FriendlyNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Bidders bids except last are lost

bchachar

Summary

If a bidder submits multiple bids the Map data structure will preserve his last bid against his address and all the earlier bids could not be tracked and hence when the auction is over his bids can not be withdrawn as they are not saved inside the contract.

Vulnerability Details

As you can see from the following test, the user bids 3 times but only his last bid is stored inside the map and his earlier bids are not saved.

#[test]
fn test_bids_lost() {
let (auction_dispatcher, auction_contract, erc20_contract_address, _) =
deploy_auction_contract();
//The owner calls the start function and the auction begins.
auction_dispatcher.start(86400, 10);
​
let erc20_dispatcher = IMockERC20TokenDispatcher { contract_address: erc20_contract_address };
//Change the caller address
let first_bidder_address: ContractAddress = 123.try_into().unwrap();
start_cheat_caller_address_global(first_bidder_address);
​
erc20_dispatcher.mint(first_bidder_address, 60);
// let balance = erc20_dispatcher.token_balance_of(first_bidder_address);
// assert(balance == 20, 'Balance must be 20');
​
erc20_dispatcher.token_approve(auction_contract, 60);
​
//The first bidder calls the bid function with amount of 11.
auction_dispatcher.bid(15);
​
let values = load(
auction_contract,
map_entry_address(
selector!("bid_values"), array![first_bidder_address.try_into().unwrap()].span(),
),
3,
);
println!("Values for first_bidder {:?}", values.span());
​
auction_dispatcher.bid(20);
​
let values = load(
auction_contract,
map_entry_address(
selector!("bid_values"), array![first_bidder_address.try_into().unwrap()].span(),
),
3,
);
println!("Values for first_bidder {:?}", values.span());
​
auction_dispatcher.bid(25);
​
stop_cheat_caller_address_global();
​
let values = load(
auction_contract,
map_entry_address(
selector!("bid_values"), array![first_bidder_address.try_into().unwrap()].span(),
),
3,
);
println!("Values for first_bidder {:?}", values.span());
}

Output

Values for first_bidder [15, 0, 0]
Values for first_bidder [20, 0, 0]
Values for first_bidder [25, 0, 0]
[PASS] starknet_auction_integrationtest::test_contract::test_bids_lost (gas: ~1996)
Running 0 test(s) from src/

Impact

The bidder will not be able to withdraw his bids other than the last one when the auction is ended.

Tools Used

Tests

Recommendations

Instead of saving current bid against sender address in map, sum of current bid and previous bid should be saved inside map against sender. This way we can keep track of total amount that someone has bid and when the auction is ended we can return this amount for all the bidders and for the winner we can subtract highest bidding from this amount and return the remaining amount to winner in case if he has bidded multiple times.

Other functions in the contract need to be changed as well.

fn bid(ref self: ContractState, amount: u64) {
let time = get_block_timestamp();
let erc20_dispatcher = IERC20Dispatcher { contract_address: self.erc20_token.read() };
let sender = get_caller_address();
let receiver = get_contract_address();
let current_bid = self.highest_bid.read();
assert(self.started.read(), 'Auction is not started');
assert(time < self.bidding_end.read(), 'Auction ended');
assert(amount > current_bid, 'The bid is not sufficient');
+ let old_bid = self.bid_values.read(sender);
+ self.bid_values.entry(sender).write(amount + old_bid);
- self.bid_values.entry(sender).write(amount);
self.emit(NewHighestBid {amount: self.highest_bid.read(), sender: sender});
self.highest_bidder.write(sender);
self.highest_bid.write(amount);
erc20_dispatcher.transfer(receiver, amount.into());
}
Output of same test with obove modification.
Values for first_bidder [15, 0, 0]
Values for first_bidder [35, 0, 0]
Values for first_bidder [60, 0, 0]
[PASS] starknet_auction_integrationtest::test_contract::test_bids_lost (gas: ~2000)
Updates

Lead Judging Commences

bube Lead Judge 8 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Wrong bid amount in `bid` function

In the `bid` function the bid values are stored using `self.bid_values.entry(sender).write(amount)` directly, but this overwrites any previous bids made by the same bidder. Therefore if a participant makes 2 or more bids, the participant can then withdraw only the last value of the last bid. That is incorrect, the protocol should save all bids and a participant should withdraw the value of the all unsuccessful bids.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.