Auction without repossessor can result in _closeAuction() revert

Summary

The solution allows to initialise new auction by any user. Action instance can be created with COMPONENT_ROLE role assigned or without. The input parameters has no strict input validation. Thus, an user can initialise new auction without setting address for repossessor parameter. In such case, when no bids were placed for particular token Id, the _closeAuction() will revert in every case, as it attempts to mint/transfer ERC721-compliant StewardLicense token to address(0), which is not allowed in OpenZepplin implementation.

Vulnerability Detail

The solution allows to initialise new auction with or without owner set. If owner is provided, the COMPONENT_ROLE role is assigned to this address. User with this role assigned can update the repossessor with the setRepossessor() function. Additionally, the repossessor has no input validation in the initialise() functions. Thus, is possible to create new audit with repossessor set to address(0) and without possibility to update this parameter.

    function initializeAuction(
        address repossessor_,
...
    ) external {
        require(
            !_isInitialized(),
            'EnglishPeriodicAuctionFacet: already initialized'
        );

        _setSupportsInterface(type(IPeriodicAuctionReadable).interfaceId, true);
        _initializeAuction(
            repossessor_,
...
        );
    }

    /**
     * @notice Initialize auction parameters with owner
     */
    function initializeAuction(
        address owner_,
        address repossessor_,
...
    ) external {
        require(
            !_isInitialized(),
            'EnglishPeriodicAuctionFacet: already initialized'
        );

        _setSupportsInterface(type(IPeriodicAuctionReadable).interfaceId, true);
        _setSupportsInterface(type(IPeriodicAuctionWritable).interfaceId, true);
        _grantRole(COMPONENT_ROLE, owner_);
        _initializeAuction(
            repossessor_,
...
        );
    }

    function _initializeAuction(
        address repossessor,
...
    ) internal {
...
        _setRepossessor(repossessor);
...
    }

    function _setRepossessor(address repossessor) internal {
        EnglishPeriodicAuctionStorage.layout().repossessor = repossessor;

        emit RepossessorSet(repossessor);
    }

    function setRepossessor(
        address repossessor_
    ) external onlyRole(COMPONENT_ROLE) {
        _setRepossessor(repossessor_);
    }

When there is no bids and auction time is ended, every call to the _closeAuction() will revert, as the function attempts to mint/transfer NFT token to address(0), which is by design disallowed in OpenZepplin implementation.

    function _closeAuction(uint256 tokenId) internal {
...
        if (l.highestBids[tokenId][currentAuctionRound].bidder == address(0)) {
            // No bids were placed, transfer to repossessor
            Bid storage repossessorBid = l.bids[tokenId][currentAuctionRound][
                l.repossessor
            ];
            repossessorBid.bidAmount = 0;
            repossessorBid.feeAmount = 0;
            repossessorBid.collateralAmount = 0;
            repossessorBid.bidder = l.repossessor;

            l.highestBids[tokenId][currentAuctionRound] = repossessorBid;
        } else if (
...
        }
...
        // Transfer to highest bidder
        IStewardLicense(address(this)).triggerTransfer(
            oldBidder,
            l.highestBids[tokenId][currentAuctionRound].bidder,
            tokenId
        );
...
    }

Proof of concept

    it('cannot close auction with no bids and repossesor set to address(0)', async function () {
      // Auction start: Now - 200
      // Auction end: Now + 100
      const instance = await getInstance({
        auctionLengthSeconds: 300,
        initialPeriodStartTime: (await time.latest()) - 200,
        licensePeriod: 1000,
        repossessor: ethers.constants.AddressZero
      });
      const licenseMock = await ethers.getContractAt(
        'NativeStewardLicenseMock',
        instance.address,
      );

      await time.increase(100);

      //@audit this call reverts
      await instance.connect(owner).closeAuction(0);
    });

Impact

The invalid configuration can result in permanent lockout of NFT due to revert in _closeAuction() function when no bids were placed.

Code Snippet

EnglishPeriodicAuctionInternal.sol#L493

Tool used

Manual Review

Recommendation

It is recommended to implement strict input validation that prevents auction creation with invalid configuration.

sherlock-audit / 2024-02-radicalxchange-judging

cocacola - Auction without repossessor can result in _closeAuction() revert #70