Losing bidder can double their money #1130
Labels
3 (High Risk)
Assets can be stolen/lost/compromised directly
bug
Something isn't working
duplicate-1323
satisfactory
satisfies C4 submission criteria; eligible for awards
Lines of code
https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/AuctionDemo.sol#L104
https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/AuctionDemo.sol#L124
https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/AuctionDemo.sol#L105
https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/AuctionDemo.sol#L125
Vulnerability details
Bug Description
The
claimAuction()
andcancelBid()
functions in AuctionDemo.sol have no re-entrancy protection and loose require checks allowing the auction to be claimed and losing bids repaid if the block timestamp is the last second of the auction.AuctionDemo.sol Lines 104-120
AuctionDemo.sol Lines 124-130
If the winner claims the auction on the last second of the auction re-entry is possible from
claimAuction()
tocancelBid()
. Any user with a losing bid will have their ETH refunded and they can use the ETH call to re-entercancelBid()
. AsclaimAuction()
setsauctionClaim[_tokenid] = true;
this can only happen once so the attacker will need to have multiple bids they can re-enter multiple times.If the auction is claimed or engineered (collusion with block builders) to finish on the last second the winner can claim the auction and each of the attacker's bids will be refunded allowing them to re-enter and
cancelBid()
doubling their money. There's more nuance to the attack wich I will cover in the Proof of Concept.Impact
This attack drains ETH from the AuctionDemo.sol contract and either valid bidders or the owner of the auction token will not receive their ETH as the contract will have no funds to pay. If the attack is executed correctly each of the attacker's losings bid will be returned twice. This is ETH that would be used to repay other bidders that still have valid bids stored in the system. The attacker steals their ETH and doubles their own.
Whether you accept block.timestamp can be timed or collusion with a block builder it should be noted that this attack can be performed at no risk to the attacker. They either double their money or they get their money back.
Proof of Concept
For maximum ETH extraction the attacker should front run each victim bid with a lower ETH value. If they see a 4ETH bid in the mempool they should bid slightly less. The earlier these bids are registered within
auctionInfoData
the better, as these are transferred earlier when the auction is claimed.If any of these victim bids are cancelled the attacker should cancel their corresponding front-run bid. The attacker wants to make sure there's always enough ETH in the contract to return their bid twice. Note though there's no risk here, the attacker will have either 1) Their original bid returned or 2) Their original bid and their cancelled bid.
This is demonstrated in the test
test_Loser_FrontRuns_Wins()
below;receive()
function.claimAuction()
is called and the block.timestamp is the last second of the auction period.claimAuction()
. Notesuccess
is returned but not checked or reverted. NB: The status of the bid is not set to false socancelBid()
is still a viable re-entry target.receive()
function is called on the attack contract andcancelBid()
is re-entered. The losing bid is returned again to the attacker.Tools Used
Vim
Recommended Mitigation Steps
The protocol should implement re-entrancy protection modifiers on the
claimAuction()
andcancelBid()
functions in AuctionDemo.sol via something like OpenZeppelin'sReentrancyGuard
for all state changing functions.Check the
success
and if it's not true then revert inclaimAuction()
;Reverting on
success
opens up other attack surfaces. The protocol could look at converting all bids from ETH to WETH withinparticipateToAuction()
to avoid direct ETH calls.Tighten the require statements in
claimAuction()
andcancelBid()
so that there is no single timestamp where you can perform both functions. For example;Change
cancelBid()
to be < rather than <=.As each bid is processed in
claimAuction()
it's worth setting the bid's state tofalse
which would have prevented the re-entrancy viacancelBid()
as that function requires the status of the bid to be true.Assessed type
Reentrancy
The text was updated successfully, but these errors were encountered: