gkelis - Winning bid funds transfer is not checked for success or failure, so that the `_auction.amount` can be lost.

[sherlock-admin2]

gkelis

high

Winning bid funds transfer is not checked for success or failure, so that the _auction.amount can be lost.

Summary

Function _safeTransferETHWithFallback transfers the funds of winning bid to DAO, but does not return any boolean value. If the transfer fails, funds will never be added to DAO.

Vulnerability Detail

1. When an auction is settled by NounsAuctionHouseV2.sol::_settleAuction, the winning bid is payed by NounsAuctionHouseV2.sol::_safeTransferETHWithFallback. nouns-contracts/contracts/NounsAuctionHouseV2.sol#L288

   if (_auction.amount > 0) {
   @>      _safeTransferETHWithFallback(owner(), _auction.amount);
   }

2. _safeTransferETHWithFallback is void, there is no boolean value returned. In case the transfer of the funds fails, the auction is settled normally, but without any funds entering DAO. nouns-contracts/contracts/NounsAuctionHouseV2.sol#L304-L309

   function _safeTransferETHWithFallback(address to, uint256 amount) internal {
       if (!_safeTransferETH(to, amount)) {
           IWETH(weth).deposit{ value: amount }();
           IERC20(weth).transfer(to, amount);
       }
   }

3. This issue is also valid for the contract NounsAuctionHouse.sol, instead of NounsAuctionHouseV2.sol, which is described here.
   Everything is the same, but the links are: https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/NounsAuctionHouse.sol#L236-L238, https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/NounsAuctionHouse.sol#L246-L251

Impact

Auction winning bid funds getting lost, if the transfer fails.

Code Snippet

https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/NounsAuctionHouseV2.sol#L287-L289

Tool used

Manual Review

Recommendation

1. Change NounsAuctionHouseV2.sol::_safeTransferETHWithFallback to return a boolean.

   -    function _safeTransferETHWithFallback(address to, uint256 amount) internal {
   +    function _safeTransferETHWithFallback(address to, uint256 amount) internal returns(bool) {
        if (!_safeTransferETH(to, amount)) {
            IWETH(weth).deposit{ value: amount }();
   -            IERC20(weth).transfer(to, amount);
   +            return IERC20(weth).transfer(to, amount);
        }
   +        return true;
    }

2. Check if _safeTransferETHWithFallback has succeeded, by a require or revert.

   if (_auction.amount > 0) {
   -       _safeTransferETHWithFallback(owner(), _auction.amount);
   +       bool success = _safeTransferETHWithFallback(owner(), _auction.amount);
   +       require(success, "Bid funds transfer failed.");
   }

[sherlock-admin2]

3 comment(s) were left on this issue during the judging contest.

WangAudit commented:

unfotunately have to grant it as low/info since the transfer reverts if fails and it's essentially a sanity check

WangAudit commented:

seems to be working as intended and the report is best practice; the problem is that the forking will be announced upfront and users will understand when the proposals should be created

karanctf commented:

low