Root + Impact
Description
-
The BidBeastsNFTMarketPlace.withdrawAllFailedCredits function tries to allow users to withdraw funds that failed to be transferred directly
-
However, it doesn't leverage the _receiver parameter to let user specify the recovery address for receiving credits, instead, it uses the exact same pattern as the _payout() function, which has already failed
function _payout(address recipient, uint256 amount) internal {
if (amount == 0) return;
(bool success, ) = payable(recipient).call{value: amount}("");
if (!success) {
failedTransferCredits[recipient] += amount;
}
}
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
Impact: High
Proof of Concept
Add the following test, then run this command: forge test --match-test test_neverRefunded -vv
function test_neverRefunded() public {
vm.deal(address(rejector), STARTING_BALANCE);
_mintNFT();
_listNFT();
vm.prank(address(rejector));
market.placeBid{value: MIN_PRICE + 1 ether}(TOKEN_ID);
vm.prank(BIDDER_1);
market.placeBid{value: MIN_PRICE + 2 ether}(TOKEN_ID);
console.log("rejector failed credits:", market.failedTransferCredits(address(rejector)));
vm.prank(address(rejector));
vm.expectRevert("Withdraw failed");
market.withdrawAllFailedCredits(address(rejector));
console.log("rejector still failes to withdraw");
console.log("rejector failed credits:", market.failedTransferCredits(address(rejector)));
}
PoC Results:
forge test --match-test test_neverRefunded -vv
[⠊] Compiling...
[⠘] Compiling 1 files with Solc 0.8.20
[⠃] Solc 0.8.20 finished in 805.73ms
Compiler run successful!
Ran 1 test for test/BidBeastsMarketPlaceTest.t.sol:BidBeastsNFTMarketTest
[PASS] test_neverRefunded() (gas: 333766)
Logs:
rejector failed credits: 2000000000000000000
rejector still failes to withdraw
rejector failed credits: 2000000000000000000
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.05ms (388.50µs CPU time)
Ran 1 test suite in 312.31ms (1.05ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Recommended Mitigation
Let users specify the recovery address via _receiver parameter
function withdrawAllFailedCredits(address _receiver) external {
uint256 amount = failedTransferCredits[_receiver];
require(amount > 0, "No credits to withdraw");
failedTransferCredits[msg.sender] = 0;
+ (bool success, ) = payable(_receiver).call{value: amount}("");
- (bool success, ) = payable(msg.sender).call{value: amount}("");
require(success, "Withdraw failed");
}
