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

Multiple bids in `bid` overwrite previous user's bids leading to loss of funds in `withdraw`.

v1vah0us3

Vulnerability Details:

The smart contract's bid function fails to handle multiple bids from the same user appropriately. When a user places multiple bids, the contract stores only the latest bid, overwriting any previous bids in the bid_values mapping.

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');
@> 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());
}

This code stores only the latest bid in the mapping, overwriting any prior bids made by the same user, instead of adding them together.

Withdraw Function Flaw:

fn withdraw(ref self: ContractState) {
assert(self.started.read(), 'Auction is not started');
assert(self.ended.read(), 'Auction is not ended');
let caller = get_caller_address();
let sender = get_contract_address();
let erc20_dispatcher = IERC20Dispatcher { contract_address: self.erc20_token.read() };
@> let amount = self.bid_values.entry(caller).read();
let amount_owner = self.highest_bid.read();
if caller == self.nft_owner.read() {
self.highest_bid.write(0);
erc20_dispatcher.transfer_from(sender, caller, amount_owner.into());
}
​
if amount > 0 {
let sender = get_contract_address();
@> erc20_dispatcher.transfer_from(sender, caller, amount.into());
}
​
self.emit(Withdraw {amount: amount, caller: caller});
}

The amount variable only contains the last bid for withdrawal, so if a user has placed multiple bids and does not win the auction, they can only withdraw the last bid amount and not the total of all their bids, losing their previous contributions.

Impact:

This vulnerability can result in loss of funds for users who place multiple bids in the auction. Users can only recover the last bid amount they placed, while their earlier bids are irrecoverable.

Tools Used

Manual Code Review, VSCode

Recommendations:

Ensure that each user's bid is added to their previous bids rather than overwriting them, allowing users to withdraw the full amount if they lose the auction. Consider implementing the following changes to the bid function:

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');
- self.bid_values.entry(sender).write(amount);
+ let previous_bid = self.bid_values.entry(sender).read();
+ let total_bid_amount = previous_bid + amount;
+ self.bid_values.entry(sender).write(total_bid_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());
}
Updates

Lead Judging Commences

bube Lead Judge 9 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.