In NounsAuctionHouseFork.sol, Fixed Amount of Gas Sent in Call May Be Insufficient

[code423n4]

Lines of code

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/fork/newdao/NounsAuctionHouseFork.sol#L272-L275

Vulnerability details

Impact

In NounsAuctionHouseFork.sol, _safeTransferETH() function is used to transfer the ETH

File: nouns-contracts/contracts/governance/fork/newdao/NounsAuctionHouseFork.sol

272    function _safeTransferETH(address to, uint256 value) internal returns (bool) {
273        (bool success, ) = to.call{ value: value, gas: 30_000 }(new bytes(0));
274        return success;
275    }

The issue here is at L-273, _safeTransferETH() makes a call with a fixed amount of gas, 30,000. If the receiver is a contract this may be insufficient to process the receive() function. As a result the user will not be able to receive funds from this function. Due to this issue, createBid() and _settleAuction() functions will largely suffer. Therefore it is recommended to remove the gas field at L-273.

Proof of Concept

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/fork/newdao/NounsAuctionHouseFork.sol#L272-L275

References

There is an exactly similar Medium severity found in Joyn audit: Reference link

Tools Used

Manual review

Recommended Mitigation Steps

Consider removing the gas field to use the default amount.

Assessed type

Other

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[eladmallel]

this is actually a feature, and we want to make it even better, see here: nounsDAO/nouns-monorepo#556.

without a gas limitation, a malicious bidder can bid from a contract that incurs huge gas costs on the next bidder when in their tx auction house tries to refund the malicious bidder.

[c4-sponsor]

eladmallel marked the issue as sponsor disputed

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[0xRizwan]

@eladmallel @gzeon-c4

With due respect to judge decision, I would like to give below additional context,

The amount of gas that is required for a call depends on a number of factors, including the complexity of the call. Per the sponsor comment,

without a gas limitation, a malicious bidder can bid from a contract that incurs huge gas costs on the next bidder when in their tx auction house tries to refund the malicious bidder.

If the malicious bidder can bid from contract and in that case createBid() can be restricted to Externally owned accounts(EOA) address, Putting gas limit on low level function .call is not a good solution to prevent malicious contracts. Let the .call handle with default amount. The below proposed solution(msg.sender == tx.origin) will only allow EOA. The contract can not call this function from outside as well as at the time of construction or say passing createBid() in constructor.

119     function createBid(uint256 nounId) external payable override nonReentrant {
+         require(msg.sender == tx.origin, "only EOA can call");


          // Some code


        if (lastBidder != address(0)) {
            _safeTransferETHWithFallback(lastBidder, _auction.amount);
        }


          // Some code


            emit AuctionExtended(_auction.nounId, _auction.endTime);
        }
151    }

and keeping the report recommendation same,

    function _safeTransferETH(address to, uint256 value) internal returns (bool) {
-        (bool success, ) = to.call{ value: value, gas: 30_000 }(new bytes(0));
+        (bool success, ) = to.call{ value: value }(new bytes(0));
        return success;
    }

Further to solidity documentation reference on hardcoded gas limits,

Reference link:- https://docs.soliditylang.org/en/latest/types.html

Gas limits should not be hardcoded even solidity documentation suggest same. I think the issue raised by sponsor can be fixed by above recommendation restricting contracts and the report recommendation is still holds true. It is a valid issue and should be taken in consideration.

Thank you!

[gzeon-c4]

This is an explicit, well-documented design choice of the project. While you are free to comment on their design, there are no immediate threat and can be upgraded in the future to adapt any breaking change on gas accounting.

[eladmallel]

@mohammedrizwann123 I think we're done here, just want to again stress to you that giving up on SC wallets as bidders is a HUGE compromise that is not worth the trouble here. Also please remember that if we fail to send ETH we simply send WETH which would not suffer from gas issues.

I'm really not sure why you're insisting here.

[0xRizwan]

@eladmallel @gzeon-c4

Thanks for the comments.

With due respect, I am not enforcing the recommendations, I have highlighted the hardcoded gas limit issue with references.

As mentioned by judge its a well documented design choice then it should be okay. I respect judge decision.