bots will be frontrunning removeCollateral for ERC721Pool

Summary

According to the whitepaper and source code, the NFTs as collaterals are claimed in a last in first out order. Since prices of NFTs in a collection can vary often, bots will be frontrunning each others removeCollateral transaction in the event a expensive NFT is next in queue for claim.

Vulnerability Detail

When adding collateral for ERC721Pool, each NFT id is added to a queue (stored in array). _transferFromSenderToPool(bucketTokenIds, tokenIdsToAdd_) will be called to transfer and add nft ids into the queue.

    uint256[]                     public bucketTokenIds;   // array of tokenIds added in pool buckets

    function addCollateral(
        uint256[] calldata tokenIdsToAdd_,
        uint256 index_
    ) external override nonReentrant returns (uint256 bucketLPs_) {
        PoolState memory poolState = _accruePoolInterest();

        bucketLPs_ = LenderActions.addCollateral(
            buckets,
            deposits,
            Maths.wad(tokenIdsToAdd_.length),
            index_
        );

        emit AddCollateralNFT(msg.sender, index_, tokenIdsToAdd_, bucketLPs_);

        // update pool interest rate state
        _updateInterestState(poolState, _lup(poolState.debt));

        // move required collateral from sender to pool
        _transferFromSenderToPool(bucketTokenIds, tokenIdsToAdd_);
    }

When removing collateral, the nfts will be dequeued. (Last in first out). _transferFromPoolToAddress(msg.sender, bucketTokenIds, noOfNFTsToRemove_) will be doing the transfer.

    function removeCollateral(
        uint256 noOfNFTsToRemove_,
        uint256 index_
    ) external override nonReentrant returns (uint256 collateralAmount_, uint256 lpAmount_) {
        _revertIfAuctionClearable(auctions, loans);

        PoolState memory poolState = _accruePoolInterest();

        collateralAmount_ = Maths.wad(noOfNFTsToRemove_);
        lpAmount_ = LenderActions.removeCollateral(
            buckets,
            deposits,
            collateralAmount_,
            index_
        );

        emit RemoveCollateral(msg.sender, index_, noOfNFTsToRemove_, lpAmount_);

        // update pool interest rate state
        _updateInterestState(poolState, _lup(poolState.debt));

        _transferFromPoolToAddress(msg.sender, bucketTokenIds, noOfNFTsToRemove_);
    }

    function _transferFromPoolToAddress(
        address toAddress_,
        uint256[] storage poolTokens_,
        uint256 amountToRemove_
    ) internal returns (uint256[] memory) {
        uint256[] memory tokensTransferred = new uint256[](amountToRemove_);

        uint256 noOfNFTsInPool = poolTokens_.length;

        uint8 nftType = _getArgUint8(NFT_TYPE);

        for (uint256 i = 0; i < amountToRemove_;) {
            uint256 tokenId = poolTokens_[--noOfNFTsInPool]; // start with transferring the last token added in bucket
            poolTokens_.pop();

            if (nftType == uint8(NFTTypes.STANDARD_ERC721)){
                _transferNFT(address(this), toAddress_, tokenId);
            }
            else if (nftType == uint8(NFTTypes.CRYPTOKITTIES)) {
                ICryptoKitties(_getArgAddress(COLLATERAL_ADDRESS)).transfer(toAddress_, tokenId);
            }
            else {
                ICryptoPunks(_getArgAddress(COLLATERAL_ADDRESS)).transferPunk(toAddress_, tokenId);
            }

            tokensTransferred[i] = tokenId;

            unchecked { ++i; }
        }

        return tokensTransferred;
    }

With such a way of removing collateral, users or bots will be frontrunning each others transaction when an expensive NFT is next in queue.

Impact

Users and bots will be incentivize to removeCollateral in the event a expensive NFT is next in queue and disincentivize to removeCollateral when a lower value NFT is next in queue.

Code Snippet

ERC721PoolPool.sol#75 ERC721Pool.sol#L251-L271 ERC721Pool.sol#L558-L582 ERC721Pool.sol#L318-L340 ERC721Pool.sol#L592-L623

Tool used

Manual Review

Recommendation

Recommend using a different way to distribute nfts for removeCollateral, for eg: random order.

Duplicate of #36

sherlock-audit / 2023-01-ajna-judging

koxuan - bots will be frontrunning removeCollateral for ERC721Pool #87