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

If a user bids multiple times, their previous bid value will be overwritten and funds will be lost.

tdey

Summary

If a user bids multiple times, their previous bid value will be overwritten and funds will be lost.

Vulnerability details

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

link to code

In bid function the bid value of a user is stored into a mapping bid_values :

bid_values: Map<ContractAddress, u64>,

In Cairo, when we perform two write operations on the same key in a Map, the second operation will overwrite the value set by the first. If a particular user bids twice then their bid value will get overwritten with the most recent one.

POC:
Alice bids 100USDC
=> self.bid_values.entry(Alice).write(100);
=> bid_values: Map<Alice,100> (pseudo code entry of the mapping)
Marry bids 150USDC
=> self.bid_values.entry(Marry).write(150);
=> bid_values: Map<Marry,150> (pseudo code entry of the mapping)
Alice bids 200USDC (again)
=> self.bid_values.entry(Alice).write(200);
=> bid_values: Map<Alice,200> (pseudo code entry of the mapping)

Now, the current mapping of Alice will store bid amount as 200USDC overwriting the previous amount of 100USDC. Hence Alice will loose 100USDC because there is no way to track/withdraw the previous bid.

Impact

If user bids more than once, they will loose their previous bid amount funds.

Tools used

Manual review

Recommended mitigation

  1. Use arrays to store multiple values of bid amount for a particular user

    //define array
    struct Bids {
    values: Array<u64>,
    }
    ​
    bid_values: Map<ContractAddress, Bids>
    ​
    //to access
    let bids = self.bid_values.entry(0x01).read();
    bids.values.append(100); // Add 100
    self.bid_values.entry(0x01).write(bids);
    ​
    bids.values.append(200); // Add 200
    self.bid_values.entry(0x01).write(bids);

  2. Use nested mapping

    bid_values: Map<ContractAddress, Map<u64, u64>>
    ​
    let bid_index = 0; // Track this for each address
    self.bid_values.entry(0x01).entry(bid_index).write(100);
    bid_index += 1;
    self.bid_values.entry(0x01).entry(bid_index).write(200);
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.