Summary
The NFT owner cannot retrieve back the NFT ownership if no bids are placed by the end of the auction period. The owner will be unable to execute the end function as it will revert with a No bids error message, leaving the NFT gets stuck in the contract permanently.
https://github.com/SiewLin-AIDeaBuddy/first-flight-2024-10-starknet-auction/blob/04f8a8b7f16edc7aeba06a3a6195e0257ddbefd0/src/starknet_auction.cairo#L149
Vulnerability Details
When the auction period ends, owner can call end function to transfer the NFT to the highest bidder and receive the tokens bid amount from the highest bidder via withdraw function. However, if there's no bids were placed by the end of the auction period, owner won't be able to retrieve back the NFT from the contract due to the assertion check assert(self.starting_price.read() < self.highest_bid.read(), 'No bids'); in the end function.
fn end(ref self: ContractState) {
let time = get_block_timestamp();
let caller = get_caller_address();
let erc721_dispatcher = IERC721Dispatcher { contract_address: self.erc721_token.read() };
let sender = get_contract_address();
assert(caller == self.nft_owner.read(), 'Not the nft owner');
assert(self.started.read(), 'Auction is not started');
assert(time >= self.bidding_end.read(), 'Auction is not yet ended');
assert(!self.ended.read(), 'Auction end is already called');
<@@>! assert(self.starting_price.read() < self.highest_bid.read(), 'No bids');
self.ended.write(true);
self.emit(End {highest_bid: self.highest_bid.read(), highest_bidder: self.highest_bidder.read()});
erc721_dispatcher.transfer_from(sender, self.highest_bidder.read(), self.nft_id.read().into());
}
When no bids were placed, the self.starting_price will be the same as self.highest_bid, and therefore causes the assertion at assert(self.starting_price.read() < self.highest_bid.read(), 'No bids'); to fail with No bids error. As the owner can't complete the end function and there's no other way for the owner to retrive back the NFT when no bids were placed, owner's NFT will therefore get stuck in the contract permanently.
Proof of Concept:
Step 1: In test\test_contract.cairo, add the following test case:
#[test]
#[should_panic(expected: 'No bids')]
fn test_audit_owner_cant_get_back_nft() {
let (auction_dispatcher, auction_contract, _, erc721_contract_address) = deploy_auction_contract();
let erc721_dispatcher = IERC721Dispatcher { contract_address: erc721_contract_address };
let time = get_block_timestamp();
auction_dispatcher.start(86400, 10);
start_cheat_block_timestamp(auction_contract, time + 86401);
auction_dispatcher.end();
stop_cheat_block_timestamp(auction_contract);
}
Step 2: Run snforge test --features enable_for_tests test_audit_owner_cant_get_back_nft
...
Collected 1 test(s) from starknet_auction package
Running 1 test(s) from tests/
[PASS] starknet_auction_integrationtest::test_contract::test_audit_owner_cant_get_back_nft (gas: ~1680)
Running 0 test(s) from src/
Tests: 1 passed, 0 failed, 0 skipped, 0 ignored, 17 filtered out
The test passes with expected panic of No bids via #[should_panic(expected: 'No bids')]
Impact
Owner can't retrive back the NFT when no bids are placed by the end of the auction period.
Tools Used
Manual review
Recommendations
To make amendment on the assertion check as follow so owner will still be able to get back the NFT in the event that no bids are placed by end of auction period
fn end(ref self: ContractState) {
let time = get_block_timestamp();
let caller = get_caller_address();
let erc721_dispatcher = IERC721Dispatcher { contract_address: self.erc721_token.read() };
let sender = get_contract_address();
assert(caller == self.nft_owner.read(), 'Not the nft owner');
assert(self.started.read(), 'Auction is not started');
assert(time >= self.bidding_end.read(), 'Auction is not yet ended');
assert(!self.ended.read(), 'Auction end is already called');
- assert(self.starting_price.read() < self.highest_bid.read(), 'No bids');
+ if (self.starting_price.read() < self.highest_bid.read()) {
+ assert(self.starting_price.read() < self.highest_bid.read(), 'No bids');
+ }
self.ended.write(true);
self.emit(End {highest_bid: self.highest_bid.read(), highest_bidder: self.highest_bidder.read()});
erc721_dispatcher.transfer_from(sender, self.highest_bidder.read(), self.nft_id.read().into());
}
Modify earlier test case to verify that the contract doesn't hold the NFT with the amendment suggested above when owner calls the end function even when there's no bids were placed by the end of the auction period.
#[test]
fn test_audit_owner_cant_get_back_nft() {
let (auction_dispatcher, auction_contract, _, erc721_contract_address) = deploy_auction_contract();
let erc721_dispatcher = IERC721Dispatcher { contract_address: erc721_contract_address };
let time = get_block_timestamp();
auction_dispatcher.start(86400, 10);
start_cheat_block_timestamp(auction_contract, time + 86401);
start_cheat_caller_address_global(auction_contract);
erc721_dispatcher.approve(auction_contract, 1);
stop_cheat_caller_address_global();
let nft_balance_before_end = erc721_dispatcher.balance_of(auction_contract);
assert(nft_balance_before_end == 1, 'Nft balance must be 1');
auction_dispatcher.end();
let nft_balance_after_end = erc721_dispatcher.balance_of(auction_contract);
assert(nft_balance_after_end == 0, 'Nft balance must be 0');
stop_cheat_block_timestamp(auction_contract);
}
Run the test snforge test --features enable_for_tests test_audit_owner_cant_get_back_nft and it will pass
....
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_owner_cant_get_back_nft (gas: ~1769)
Tests: 1 passed, 0 failed, 0 skipped, 0 ignored, 17 filtered out
