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

Bid Overwriting and Lack of Incremental Bidding

shellh4wks

Bid Overwriting and Lack of Incremental Bidding

Severity: High

Description: The current implementation overwrites previous bids from the same user instead of accumulating them. This leads to a loss of bid history and potential loss of funds for users who place multiple bids.

Impact:

  • Users who place multiple bids lose their previous bid amounts.

  • The contract fails to accurately track the total amount bid by each user.

  • There's no mechanism to refund previous bids if a user is outbid or the auction ends without them winning.

  • This creates confusion and potential financial loss for users who incrementally increase their bids.

Code Part:

fn bid(ref self: ContractState, amount: u64) {
// ... (existing checks)
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());
}

Recommendation: Modify the bid function to accumulate bids and implement a refund mechanism for previous bids. Here's a proposed fix:

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_highest_bid = self.highest_bid.read();
let previous_user_bid = self.bid_values.entry(sender).read();
​
assert(self.started.read(), 'Auction is not started');
assert(time < self.bidding_end.read(), 'Auction ended');
assert(amount + previous_user_bid > current_highest_bid, 'Bid not sufficient');
​
// Accumulate the new bid with any previous bids
let total_user_bid = previous_user_bid + amount;
self.bid_values.entry(sender).write(total_user_bid);
​
// Update highest bid if this total is now the highest
if total_user_bid > current_highest_bid {
self.highest_bidder.write(sender);
self.highest_bid.write(total_user_bid);
self.emit(NewHighestBid { amount: total_user_bid, sender });
}
​
// Transfer only the new bid amount to the contract
erc20_dispatcher.transfer(receiver, amount.into());
}

These changes ensure that:

  1. Multiple bids from the same user are accumulated rather than overwritten.

  2. Users can incrementally increase their bids without losing previous amounts.

  3. The contract accurately tracks the total amount bid by each user.

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.