gkelis - Not winning bid funds refund is not checked for success or failure, so that any previous bid `_auction.amount` can be lost.

[sherlock-admin3]

gkelis

medium

Not winning bid funds refund is not checked for success or failure, so that any previous bid _auction.amount can be lost.

Summary

Function _safeTransferETHWithFallback refunds the previous bid amount, of latest bigger one, to bidder, but does not return any boolean value. If the transfer fails, funds will never be refunded.

Vulnerability Detail

1. When a bigger bid is created by NounsAuctionHouseV2.sol::createBid, the previous winning bid is payed back by NounsAuctionHouseV2.sol::_safeTransferETHWithFallback. nouns-contracts/contracts/NounsAuctionHouseV2.sol#L165

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

2. _safeTransferETHWithFallback is void, there is no boolean value returned. In case the transfer of the funds fails, the auction continues normally, but without any funds returned to last bidder. 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#L118-L120, https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/NounsAuctionHouse.sol#L246-L251

   Impact

Previous bid funds, can be trapped in DAO, when a bigger bid happens, if the refund transfer fails.

Code Snippet

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

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.

   +   event ClientBidRefund(address indexed lastBidder, uint256 amount, bool success);
   +
   if (lastBidder != address(0)) {
   -        _safeTransferETHWithFallback(lastBidder, _auction.amount);
   +        bool success = _safeTransferETHWithFallback(lastBidder, _auction.amount);
   +        emit ClientBidRefund(lastBidder, _auction.amount, success);
   }

A revert could be triggered here, when this transfer fails, but maybe it's too much. By emitting an event the DAO knows, if the funds were really transferred, to correct the issue.

[sherlock-admin4]

1 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