Summary
The event in StarknetAuction::bid function was found emitted before the highest_bid was updated with latest user bid amount. This causes the bid amount emitted by the event is still the previous bid amount and not the user bid amount, resulting incorrect information in the emitted event upon the successful of bid transaction.
https://github.com/SiewLin-AIDeaBuddy/first-flight-2024-10-starknet-auction/blob/04f8a8b7f16edc7aeba06a3a6195e0257ddbefd0/src/starknet_auction.cairo#L109
Vulnerability Details
In StarknetAuction::bid function, the event was found emitted before the highest_bid was updated with latest user bid amount.
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());
}
At the end of a successful bid process, the event will emit the previous bid amount and not the latest user bid amount. This will create confusion and impact any downstream/front-end process that relies on the emitted event for proper rendering of the bid transaction information.
Proof of Concept:
Step 1: Update the visibility of the following event and struct in src\starknet_auction.cairo
#[event]
#[derive(Drop, starknet::Event)]
- enum Event {
+ pub enum Event {
Started: Started,
NewHighestBid: NewHighestBid,
Withdraw: Withdraw,
End: End,
}
#[derive(Drop, starknet::Event)]
- struct NewHighestBid {
- amount: u64,
- sender: ContractAddress,
+ pub struct NewHighestBid {
+ pub amount: u64,
+ pub sender: ContractAddress,
}
Step 2: In tests\test_contract.cairo, add the following import and test
use snforge_std::{spy_events, EventSpyAssertionsTrait};
use starknet_auction::starknet_auction::StarknetAuction;
#[test]
fn test_audit_bid_event_issue() {
let (auction_dispatcher, auction_contract, erc20_contract_address, _) = deploy_auction_contract();
let erc20_dispatcher = IMockERC20TokenDispatcher { contract_address: erc20_contract_address };
let bidder: ContractAddress = 123.try_into().unwrap();
let mut spy = spy_events();
auction_dispatcher.start(86400, 10);
start_cheat_caller_address_global(bidder);
erc20_dispatcher.mint(bidder, 15);
erc20_dispatcher.token_approve(auction_contract, 15);
auction_dispatcher.bid(15);
let bidder_balance = erc20_dispatcher.token_balance_of(bidder);
assert(bidder_balance == 0, 'bidder balance should be zero');
let wrong_event_bid_amount = StarknetAuction::Event::NewHighestBid(StarknetAuction::NewHighestBid {amount: 10, sender: bidder});
spy.assert_emitted(@array![(auction_contract, wrong_event_bid_amount)]);
stop_cheat_caller_address_global();
}
Step 3: Run the test snforge test --features enable_for_tests test_audit_bid_event_issue
...
Collected 1 test(s) from starknet_auction package
Running 0 test(s) from src/
Running 1 test(s) from tests/
[PASS] starknet_auction_integrationtest::test_contract::test_audit_bid_event_issue (gas: ~1962)
Tests: 1 passed, 0 failed, 0 skipped, 0 ignored, 16 filtered out
The test passes indicating that the event is wrongly emitted with previous setup bid instead of the user latest bid amount
Impact
Misleading information that confuse user and impact downstream/front-end process that relies on the event for rendering bid transaction information
Tools Used
Manual review with test
Recommendations
Make amendment to StarknetAuction::bid function by shifting the event emit after the update of highest_bid
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);
+ self.emit(NewHighestBid {amount: self.highest_bid.read(), sender: sender});
erc20_dispatcher.transfer(receiver, amount.into());
}
