SoulReaper33 - Previous bidder will not get a refund in some cases

[sherlock-admin3]

SoulReaper33

High

Previous bidder will not get a refund in some cases

Summary

NounsAuctionHouseV3.sol::createBid uses NounsAuctionHouseV3.sol::_safeTransferETHWithFallback to refund the previous bidder , which may still fail , hence making previous bidders unable to get their refunds

Root Cause

The choice to refund previous bidders by first transferring eth , and if that fails , transferring it as weth is a mistake , as the weth transfer can also fail( and that too silently) as the users may have a malicious or missing fallback or receive function
createBid : https://github.com/sherlock-audit/2024-11-nounsdao/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/NounsAuctionHouseV3.sol#L145
_safeTransferETHWithFallback : https://github.com/sherlock-audit/2024-11-nounsdao/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/NounsAuctionHouseV3.sol#L365

Internal pre-conditions

1. A user needs to bid in an auction
2. They need to have a malicious or missing fallback and receive function

External pre-conditions

No response

Attack Path

1. A user needs to bid in an auction
2. They need to have a malicious or missing fallback and receive function
3. Another user needs to bid in the same auction , by calling the createBid function
4. This function internally calls _safeTransferETHWithFallback , which would fail for the previous bidder

Impact

The previous bidder loses out on their initial bid

PoC

In this PoC , I have simply made the fallback and receive function to revert , but keep in mind the previous bidder may have these functions wrongly implemented and/or missing also. The point is it is extra work and caution on the user's side to correctly implement their fallback and receive functions , which won't always be the case and this is where their refund will fail

In the NounsAuctionHouseV3.t.sol :

1. Add the following contract

    contract MaliciousUser {
    fallback() external payable{
        revert();
    }
    receive() external payable{
        revert();
    }
}

2. Paste the following function in NounsAuctionHouseV3Test

    function test_createBid_breaks() public {
        uint256 nounId = auction.auction().nounId;
        MaliciousUser malUser = new MaliciousUser();
        // address bidder1 = address(0x4444);
        address bidder2 = address(0x5555);

        vm.deal(address(malUser), 1.1 ether);
        vm.prank(address(malUser));
        auction.createBid{ value: 1.1 ether }(nounId);

        assertEq(address(malUser).balance, 0);

        vm.deal(bidder2, 2.2 ether);
        vm.prank(bidder2);
        auction.createBid{ value: 2.2 ether }(nounId);

        assertEq(address(malUser).balance, 0); // didnt get the refund
        assertEq(bidder2.balance, 0); // WAS successfully able to bid
    }

Mitigation

Use pull over push mechanism , i.e. , dont refund the previous bidder right away in the same function , instead store the balances(refunds) using a mapping , and create a withdraw function , where people can withdraw their funds if they are eligible for it.