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

Incorrect bid handling in bid function

abhishekthakur

Summary

The bid function overwrites previous bids instead of accumulating them, potentially leading to loss of funds for users who bid multiple times.

Vulnerability Details

https://github.com/Cyfrin/2024-10-starknet-auction/blob/main/src/starknet_auction.cairo#L97-L114

In the current implementation, when a user places a bid, the contract overwrites the previous bid amount instead of adding to it. This is due to the line:

self.bid_values.entry(sender).write(amount);

This directly sets the bid value for the sender to the new amount, discarding any previous bids they may have made.

POC

#[test]
fn test_bid_overwrite() {
let (auction_dispatcher, auction_contract, erc20_contract_address, _) = deploy_auction_contract();
auction_dispatcher.start(86400, 10);
​
let erc20_dispatcher = IMockERC20TokenDispatcher { contract_address: erc20_contract_address };
let bidder_address: ContractAddress = 123.try_into().unwrap();
start_cheat_caller_address_global(bidder_address);
erc20_dispatcher.mint(bidder_address, 30);
erc20_dispatcher.token_approve(auction_contract, 30);
auction_dispatcher.bid(15);
auction_dispatcher.bid(20);
​
stop_cheat_caller_address_global();
let bid_values = load(auction_contract, map_entry_address(selector!("bid_values"), array![bidder_address.try_into().unwrap()].span(), ), 1, );
assert(*bid_values.at(0) == 20, 'Bid should be 20, not 35');
}

Impact

Users who bid multiple times will lose the value of their previous bids, potentially resulting in significant financial loss.

Tools Used

Manual Review

Recommendations

Modify the bid function to accumulate bids for each user instead of overwriting them.

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.