Skip to content

Latest commit

 

History

History
159 lines (120 loc) · 8.66 KB

050.md

File metadata and controls

159 lines (120 loc) · 8.66 KB

Great Onyx Lemur

Medium

NounsAuctionHouseV3::_settleAuction transfers Noun to winner contract without checking for ERC721 receiver compliance, resulting in winner contract unable to cancel stream

Summary

NounAuctionHouseV3::_settleAuction transfers Noun to the winner and creates a Noun's escrow stream without checking whether the winner is an ERC721 receiver compliant contract. If the winner is a non-ERC721 compliant contract, this will cause the winner to be unable to cancel the Noun's escrow stream and receive their unvested funds when calling StreamEscrow::cancelStream. In more detail, StreamEscrow::cancelStream cancels the escrow stream by repossessing the winner's Noun via nounsToken.transferFrom, in which the non-ERC721 compliant winner contract is unable provide the prior approval for (ERC721::approve). Thus this breaks core functionality of the StreamEscrow contract stated in the audit scope doc (excerpt below).

NounsAuctionHouseV3.sol#L349

        if (_auction.bidder == address(0)) {
            nouns.burn(_auction.nounId);
        } else {
@>          nouns.transferFrom(address(this), _auction.bidder, _auction.nounId);
        }

StreamEscrow.sol#L171

    function cancelStream(uint256 nounId) public {
        require(isStreamActive(nounId), 'stream not active');

        // transfer noun to treasury
@>      nounsToken.transferFrom(msg.sender, nounsRecipient, nounId);

audit scope doc

"Any Noun owner can cancel their Noun’s stream by calling a function that returns their Noun to the DAO and transfers back to them their unvested funds."

Root Cause

From NounsAuctionHouseV3.sol#L349, NounAuctionHouseV3::_settleAuction does not check whether auction winner is an ERC721 compliant contract before transferring Noun to the winner and creating Noun's escrow stream. Hence, the Noun can be stuck in a non-ERC721 compliant contract and the stream cannot be cancelled.

Internal pre-conditions

  1. Auction must exist and is active

External pre-conditions

None

Attack Path

  1. User deploys and funds a non-ERC721 compliant bidding contract
  2. User bids auction via bidding contract
  3. Auction ends and user wins (highest bid), causing Noun to be transferred to bidding contract and Noun's escrow stream is created
  4. User/biddingContract cannot cancel stream

Impact

Impact: High. User is unable to cancel the Noun's escrow stream and receive their unvested funds (up to 80% of the bidding amount), which breaks the core functionality of StreamEscrow contract.
Likelihood: Medium. User has to bid and win an auction using a non-ERC721 compliant bidding contract
Severity: Medium.

PoC

  1. Place the bidding contract code within NounsAuctionHouseV3.t.sol
  2. Place the test function code within test contract such as NounsAuctionHouseV3.t.sol::NounsAuctionHouseV3Test.
  3. Run the test using

forge test --mt testContractBidderCannotCancelStream --ffi

Bidding contract code

import { IStreamEscrow } from "contracts/StreamEscrow.sol";
import { NounsToken } from "contracts/NounsToken.sol";

contract ContractBidder {
    NounsAuctionHouseV3 auctionHouse;
    IStreamEscrow streamEscrow;

    constructor(NounsAuctionHouseV3 _auctionHouse, IStreamEscrow _streamEscrow){
        auctionHouse = _auctionHouse;
        streamEscrow = _streamEscrow;
    }

    function bid() external {
        uint128 nounId = auctionHouse.auction().nounId;
        auctionHouse.createBid{ value: address(this).balance }(nounId);
    }

    function cancelStream(uint256 nounId) external {
        streamEscrow.cancelStream(nounId);
    }

    receive() payable external {}
}

Test function code

    function testContractBidderCannotCancelStream() external {
        address alice = makeAddr("alice");
        uint256 bidAmount = 2 ether;
        uint256 nounId = auction.auction().nounId;
        IStreamEscrow escrow = auction.streamEscrow();
        
        // Step 1: User deploys and funds biddingContract
        vm.deal(alice, bidAmount);
        vm.startPrank(alice);
        ContractBidder biddingContract = new ContractBidder(auction, escrow);
        (bool funded, ) = payable(biddingContract).call{value: bidAmount}("");
        require(funded);
        vm.stopPrank();

        // Step 2: User bids auction via biddingContract
        vm.startPrank(alice);
        biddingContract.bid();
        vm.stopPrank();

        // Step 3: Auction ends and user wins
        // Assert: Nouns transferred to biddingContract (despite biddingContract not complying with ERC721 receiver)
        // Assert: Stream is created
        endAuctionAndSettle();
        NounsToken nouns = NounsToken(address(auction.nouns()));
        assertEq(nouns.ownerOf(nounId), address(biddingContract));
        assertTrue(escrow.isStreamActive(nounId));

        // Step 4: User/biddingContract cannot cancel stream
        // biddingContract does not comply with ERC721 receiver and cannot execute ERC721::approve
        // biddingContract can not execute StreamEscrow::cancelStream as it requires ERC721::approve
        vm.startPrank(alice);
        vm.expectRevert('ERC721: transfer caller is not owner nor approved');
        biddingContract.cancelStream(nounId);
        vm.stopPrank();
    }

Mitigation

NounAuctionHouseV3 should check whether the winner is an ERC721 compliant contract before sending the Noun. This can be done using safeTransferFrom (ref) instead of transferFrom (which OpenZeppelin’s documentation discourages the use of). The safeTransferFrom first checks if the recipient (winner) is a contract and then calls onERC721Received (ref) on the recipient to check if the recipient (winner) is ERC721 compliant. Hence, this ensures that the recipient contract can perform the neccessary approval (ERC721::approve) required to cancel streams using StreamEscrow::cancelStream.

Additionally, a "pull over push" design pattern (ref) should be implemented for distributing the Noun to the winner. This is to prevent a malicious winner contract from reverting on receiving the Noun, thus preventing the settlement of an auction and creation of new auction in a Denial-of-Service Attack (DoS) (ref, ref). In detail, NounsAuctionHouseV3::_settleAuction should only register the winner and give it approval to claim the Noun, whereas the winner should call a separate function (e.g. claimNoun) to claim the Noun.

The following code changes takes into account both of the above recommendations.

        } else {
-          nouns.transferFrom(address(this), _auction.bidder, _auction.nounId);
+          nouns.approve(_auction.bidder, nounId);
        }

        SettlementState storage settlementState = settlementHistory[_auction.nounId];
        settlementState.blockTimestamp = uint32(block.timestamp);
        settlementState.amount = ethPriceToUint64(_auction.amount);
        settlementState.winner = _auction.bidder;
        if (_auction.clientId > 0) settlementState.clientId = _auction.clientId;

        emit AuctionSettled(_auction.nounId, _auction.bidder, _auction.amount);
        if (_auction.clientId > 0) emit AuctionSettledWithClientId(_auction.nounId, _auction.clientId);
    }

+   function claimNoun(uint256 nounId) external {
+       SettlementState storage settlementState = settlementHistory[nounId];
+       require(msg.sender == settlementState.winner, "Not winner, cannot claim");
+       nouns.safeTransferFrom(address(this), msg.sender, nounId);
+   }