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

[M-2] The `BidBeastsNFTMarket::withdrawAllFailedCredits` function sends funds to `msg.sender` instead of `_receiver`, failed transfer funds cannot be withdrawn leading to funds loss and denial of service (DoS).

alexscherbatyuk

The BidBeastsNFTMarket::withdrawAllFailedCredits function sends funds to msg.sender instead of _receiver, failed transfer funds cannot be withdrawn leading to funds loss and denial of service (DoS).

Description

  • The BidBeastsNFTMarket::withdrawAllFailedCredits function sends funds to `msg.sender` instead of `_receiver`, repeating the same failure path for failed transfers.

  • Failed transfer funds cannot be withdrawn. Funds loss for users. Breaking the contract's intended functionality.

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

Risk

Likelihood: Medium

  • It might occur under specific conditions

  • The direct transfer has to fail in order for this function to be included in the normal flow.

Impact: Medium

  • User funds are indirectly at risk.

  • Some level of disruption to the protocol's functionality.

Proof of Concept

Add the following code snippet to the `BidBeastsMarketPlaceTest.t.sol` test file.

function testDenailOfService() public {
uint256 S_MIN_BID_INCREMENT_PERCENTAGE = 5;
​
address SECOND_USER_ADDRESS = makeAddr("otherUser");
​
// Mint NFT to the user `BIDDER_1`
vm.startPrank(OWNER);
nft.mint(SELLER);
vm.stopPrank();
​
// The `SELLER` lists the NFT
vm.startPrank(SELLER);
nft.approve(address(market), TOKEN_ID);
market.listNFT(TOKEN_ID, MIN_PRICE, BUY_NOW_PRICE);
vm.stopPrank();
​
// The uses `rejector contract` to place a bid
uint256 firstBidAmount = MIN_PRICE + 1;
hoax(address(rejector), 100 ether);
market.placeBid{value: firstBidAmount}(TOKEN_ID);
​
// Another bidder sets a bid
uint256 secondBidAmount = firstBidAmount + (firstBidAmount * S_MIN_BID_INCREMENT_PERCENTAGE) / 100;
vm.prank(BIDDER_1);
market.placeBid{value: secondBidAmount}(TOKEN_ID);
​
// user's `rejector contract` rejects the refund intended for previousBidder address
// failed funds are added to failedTransferCredits mapping
​
assertTrue(
market.failedTransferCredits(address(rejector)) > 0, "Failed transfer credits should be greater than 0"
);
​
// user tries to withdraw failed funds to second address
vm.prank(address(rejector));
vm.expectRevert("No credits to withdraw");
market.withdrawAllFailedCredits(SECOND_USER_ADDRESS);
}

Recommended Mitigation

Modify the `BidBeastsNFTMarket::withdrawAllFailedCredits` function to send funds to `_receiver` instead of `msg.sender`.

function withdrawAllFailedCredits(address _receiver) external {
+ require(_receiver != address(0), "Receiver cannot be address(0)");
- 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 about 1 month 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.