ParaSpace contest - unforgiven's results

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L326-L338

Vulnerability details

Impact

Function _removeFeeder() in NFTFloorOracle is supposed to remove a feeder and in doing so it should update feederPositionMap[] and feeders[] variables but right it moves feeder in the last index(feeder1) with the removed feeder but it fail to update feederPositionMap[feeder].indexfor the last feeder so in the future the value offeederPositionMap[feeder1].index would be wrong and admin can't removefeeder1. this bug can cause feeders to be not removable and if some of the feeders were compromised or were providing wrong prices then admin can't remove them and the pricing of NFTs would calculated improperly. contract NFTFloorOracle would be in wrong state and the length of feeders[]list would increase over time and causesetPrice()` calls to fail because of gas and contract state become broken and won't be recoverable.

Proof of Concept

This is _removeFeeder() code in NFTFloorOracle:

    function _removeFeeder(address _feeder)
        internal
        onlyWhenFeederExisted(_feeder)
    {
        uint8 feederIndex = feederPositionMap[_feeder].index;
        if (feederIndex >= 0 && feeders[feederIndex] == _feeder) {
            feeders[feederIndex] = feeders[feeders.length - 1];
            feeders.pop();
        }
        delete feederPositionMap[_feeder];
        revokeRole(UPDATER_ROLE, _feeder);
        emit FeederRemoved(_feeder);
    }

As you can see contract moves the last feeder in the feeders[] to the place of removed feeder in the line: feeders[feederIndex] = feeders[feeders.length - 1]; but contract don't update feederPositionMap[] for that last feeder(FeederLast) so from now on admin can't remove FeederLast from feeders[] list. because the value of feederPositionMap[] is wrong and and feeders[FeederPositionMap[feederLast].index] is not FeederLast and if admin wants to remove FeederLast the value won't be removed from feeders array. imagine this scenario:

1. they are 3 feeders registered in contract: feeders[] = [feederA, feederB, feederC] and the value of feederPositionMap[] for feederA, feederB, feederC are 0, 1, 2.
2. admin calls removeFeeder(feederA) and contract move feederC to first position in feeders[] list and then pop the last element then the variables would be: feeders[] = [feederC, feederB] and feederPositionMap[feederC].index would be 2.
3. now if admin wants to remove feederC then the transaction would revert because of out of bond array index (index 2 isn't exists in feeders[] list).
4. if some other feeders get inserted to the contract and feeders[] length get bigger than 2, even then if admin wants to remove feederC then contract can't remove it from list feeders[] because of the condition of the If statement would be false (feeders[ feederPositionMap[feederC].index] != feederC).
5. contract storage is in wrong state and feederPositionMap[feederC].index isn't equal to feederC position and by removing other feeders the state would be get worse and as times goes the feeders[] list would get bigger and bigger and because some logics iterates through feeders[] to try to calculate the fair price then those logics would fail because of gas like setPrice() function.

Tools Used

VIM

Recommended Mitigation Steps

update feederPositionMap[] variable too

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/pool/PoolCore.sol#L426-L455 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/LiquidationLogic.sol#L150-L273 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/LiquidationLogic.sol#L516-L556 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/ReserveLogic.sol#L161-L207 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/pool/DefaultReserveInterestRateStrategy.sol#L126-L175

Vulnerability details

Impact

Function executeLiquidateERC20() to liquidate a position if its Health Factor drops below 1. The caller (liquidator) covers liquidationAmount amount of debt of the user getting liquidated, and receives a proportional amount of the collateralAsset plus a bonus to cover market risk. during the call contract receives liquidation asset from liquidator and then updates borrow & supply rate of that asset. but because it first transfers liquidation asset to contract then calls liquidationAssetReserve.updateInterestRates() so the calculation of the interest rates in calculateInterestRates() of DefaultReserveInterestRateStrategy would be wrong because code uses vars.availableLiquidity = IToken(params.reserve).balanceOf(params.xToken) + params.liquidityAdded - params.liquidityTaken; to calculate available liquidity but the transferred amount would calculated two times, one time in the balanceOf() and one time in params.liquidityAdded. this would cause wrong interest rates for asset and suppliers or borrowers would lose their entitled funds. the impact can be big when liquidityAdded / balanceOf(contract) is big.

Proof of Concept

This is _burnDebtTokens() code in LiquidationLogic contract which is get called during executeLiquidateERC20():

    /**
     * @notice Burns the debt tokens of the user up to the amount being repaid by the liquidator.
     * @dev The function alters the `liquidationAssetReserveCache` state in `vars` to update the debt related data.
     * @param liquidationAssetReserve The data of the liquidation reserve
     * @param params The additional parameters needed to execute the liquidation function
     * @param vars the executeLiquidateERC20() function local vars
     */
    function _burnDebtTokens(
        DataTypes.ReserveData storage liquidationAssetReserve,
        DataTypes.ExecuteLiquidateParams memory params,
        ExecuteLiquidateLocalVars memory vars
    ) internal {
        _depositETH(params, vars);

        // Transfers the debt asset being repaid to the xToken, where the liquidity is kept
        IERC20(params.liquidationAsset).safeTransferFrom(
            vars.payer,
            vars.liquidationAssetReserveCache.xTokenAddress,
            vars.actualLiquidationAmount
        );
        // Handle payment
        IPToken(vars.liquidationAssetReserveCache.xTokenAddress)
            .handleRepayment(params.liquidator, vars.actualLiquidationAmount);
        // Burn borrower's debt token
        vars
            .liquidationAssetReserveCache
            .nextScaledVariableDebt = IVariableDebtToken(
            vars.liquidationAssetReserveCache.variableDebtTokenAddress
        ).burn(
                params.borrower,
                vars.actualLiquidationAmount,
                vars.liquidationAssetReserveCache.nextVariableBorrowIndex
            );
        // Update borrow & supply rate
        liquidationAssetReserve.updateInterestRates(
            vars.liquidationAssetReserveCache,
            params.liquidationAsset,
            vars.actualLiquidationAmount,
            0
        );
    }

As you can see it first transfers actualLiquidationAmount amount of liquidationAsset from payer to xTokenAddress then in the end it calls: updateInterestRates(,liquidationAsset,actualLiquidationAmount,0) to update interest rate for liquidationAsset and actualLiquidationAmount is sent as liquidityAdded to updateInterestRates() function, so when updateInterestRates() is get called the liquidityAdded amount of liquidationAsset is already in xTokenAddress balance. This is updateInterestRates() code:

   /**
     * @notice Updates the reserve current stable borrow rate, the current variable borrow rate and the current liquidity rate.
     * @param reserve The reserve reserve to be updated
     * @param reserveCache The caching layer for the reserve data
     * @param reserveAddress The address of the reserve to be updated
     * @param liquidityAdded The amount of liquidity added to the protocol (supply or repay) in the previous action
     * @param liquidityTaken The amount of liquidity taken from the protocol (redeem or borrow)
     **/
    function updateInterestRates(
        DataTypes.ReserveData storage reserve,
        DataTypes.ReserveCache memory reserveCache,
        address reserveAddress,
        uint256 liquidityAdded,
        uint256 liquidityTaken
    ) internal {
        UpdateInterestRatesLocalVars memory vars;

        vars.totalVariableDebt = reserveCache.nextScaledVariableDebt.rayMul(
            reserveCache.nextVariableBorrowIndex
        );

        (
            vars.nextLiquidityRate,
            vars.nextVariableRate
        ) = IReserveInterestRateStrategy(reserve.interestRateStrategyAddress)
            .calculateInterestRates(
                DataTypes.CalculateInterestRatesParams({
                    liquidityAdded: liquidityAdded,
                    liquidityTaken: liquidityTaken,
                    totalVariableDebt: vars.totalVariableDebt,
                    reserveFactor: reserveCache.reserveFactor,
                    reserve: reserveAddress,
                    xToken: reserveCache.xTokenAddress
                })
            );

        reserve.currentLiquidityRate = vars.nextLiquidityRate.toUint128();
        reserve.currentVariableBorrowRate = vars.nextVariableRate.toUint128();

        emit ReserveDataUpdated(
            reserveAddress,
            vars.nextLiquidityRate,
            vars.nextVariableRate,
            reserveCache.nextLiquidityIndex,
            reserveCache.nextVariableBorrowIndex
        );
    }

As you can see it just passes parameters to calculateInterestRates() to calculate interest rates and set them. this is calculateInterestRates() code:

    /// @inheritdoc IReserveInterestRateStrategy
    function calculateInterestRates(
        DataTypes.CalculateInterestRatesParams calldata params
    ) external view override returns (uint256, uint256) {
        CalcInterestRatesLocalVars memory vars;

        vars.totalDebt = params.totalVariableDebt;

        vars.currentLiquidityRate = 0;
        vars.currentVariableBorrowRate = _baseVariableBorrowRate;

        if (vars.totalDebt != 0) {
            vars.availableLiquidity =
                IToken(params.reserve).balanceOf(params.xToken) +
                params.liquidityAdded -
                params.liquidityTaken;

            vars.availableLiquidityPlusDebt =
                vars.availableLiquidity +
                vars.totalDebt;
            vars.borrowUsageRatio = vars.totalDebt.rayDiv(
                vars.availableLiquidityPlusDebt
            );
            vars.supplyUsageRatio = vars.totalDebt.rayDiv(
                vars.availableLiquidityPlusDebt
            );
        }

        if (vars.borrowUsageRatio > OPTIMAL_USAGE_RATIO) {
            uint256 excessBorrowUsageRatio = (vars.borrowUsageRatio -
                OPTIMAL_USAGE_RATIO).rayDiv(MAX_EXCESS_USAGE_RATIO);

            vars.currentVariableBorrowRate +=
                _variableRateSlope1 +
                _variableRateSlope2.rayMul(excessBorrowUsageRatio);
        } else {
            vars.currentVariableBorrowRate += _variableRateSlope1
                .rayMul(vars.borrowUsageRatio)
                .rayDiv(OPTIMAL_USAGE_RATIO);
        }

        vars.currentLiquidityRate = vars
            .currentVariableBorrowRate
            .rayMul(vars.supplyUsageRatio)
            .percentMul(
                PercentageMath.PERCENTAGE_FACTOR - params.reserveFactor
            );

        return (vars.currentLiquidityRate, vars.currentVariableBorrowRate);
    }

You can see that in the line vars.availableLiquidity = IToken(params.reserve).balanceOf(params.xToken) + params.liquidityAdded - arams.liquidityTaken; to calculate availableLiqudity contract sums current balance and liquidityAdded values but the liquidityAdded amount already is in the contract balance and it would be added two times which would make all the calculations of the interest rate to go wrong. if liquidityAdded was big portion of the current balance then the calculation would be very wrong this bug would cause to suppliers to lose funds over time because contract overestimates the available liquidity in the pool and even attacker can manipulate the interest rate by liquidation of huge amount. This bug would happen during all the liquidations and over time suppliers receive less interest rate than they would suppose to get and borrowers would pay less interest rate than they would suppose to give. and also the curve where interest rate moves on it based on supply and borrow amount would be different and it can cause total failure of risk management and borrow and supply balance of the system(users tend to borrow and when liquidation happens interest rate would decrease and users would borrow more and more which would result in more borrow -> more liquidation -> less borrow rate -> more borrow.....).

Tools Used

VIM

Recommended Mitigation Steps

transfer tokens after calling to liquidationAssetReserve.updateInterestRates().

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L165-L172

Vulnerability details

Impact

function removeFeeder() removes feeder and it supposed to be only called by owner, feeders provide price for NFTs and they are whitelisted and providing those prices are essential for protocol well functioning. but function removeFeeder() doesn't have proper modifiers and anyone can call it, so attacker can remove all the feeders(or keep the bad feeders and remove the good ones) and cause protocol to be in broken state(constantly remove feeders, it can be done by front-run for feeder's transactions) or in the state where prices are wrong. the price of NFTs won't be updated or be wrong and collaterals would be frozen or be liquidated.

Proof of Concept

This is removeFeeder() and _removeFeeder() code in NFTFloorOracle:

    /// @notice Allows owner to remove feeder.
    /// @param _feeder feeder to remove
    function removeFeeder(address _feeder)
        external
        onlyWhenFeederExisted(_feeder)
    {
        _removeFeeder(_feeder);
    }

    function _removeFeeder(address _feeder)
        internal
        onlyWhenFeederExisted(_feeder)
    {
        uint8 feederIndex = feederPositionMap[_feeder].index;
        if (feederIndex >= 0 && feeders[feederIndex] == _feeder) {
            feeders[feederIndex] = feeders[feeders.length - 1];
            feeders.pop();
        }
        delete feederPositionMap[_feeder];
        revokeRole(UPDATER_ROLE, _feeder);
        emit FeederRemoved(_feeder);
    }

As you can see there is no modifier onlyRole(DEFAULT_ADMIN_ROLE) to make sure that those function only callable by admin so anyone can call it and remove the feeders. the NFTFloorOracle contract has an algorithm to calculate price of assets and it requires feeders to provide prices and then it select price between unexpired raw prices by selecting the median price. if the number of feeders that provided prices are below a threshold then algorithm won't update the price. so attacker can remove feeders from contract(constantly when they are added) and make all the pricing of NFT's in the ParaSpace protocol to be broken and by doing so, all the functionalities of the protocol would not work. in another scenario attacker who has control over some of the feeders(or some feeders work in a wrong way), attacker would remove good feeders and only keep bad feeders who has control over them, then attacker can control the price of NFTs and users would lose a lot of funds.

Tools Used

VIM

Recommended Mitigation Steps

add proper modifier for the function so it can only be called by admin.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/tokenization/base/MintableIncentivizedERC721.sol#L303-L327

Vulnerability details

Impact

Contract MintableIncentivizedERC721 which is supposed to be and ERC721 token, didn't properly implement safeTransferFrom() and safeTransfer() logics and it doesn't check first that contract recipients are aware of the ERC721 protocol to prevent tokens from being forever locked. all other ERC721 tokens in the protocol(NToken*) are child contract of this contract and all of them won't be a valid ERC721 contract. with this bug other protocols wouldn't be able to work with Para tokens. This is High risk issue because the protocol doesn't have compliance with basic and common standards and the second reason is that all of the ERC721 tokens in the protocol would have this issue.

Proof of Concept

function safeTransferFrom() and safeTransfer() in their calls, in the end call _safeTransfer(). This is safeTransferFrom() and _safeTransfer() and _safeTransferFrom() code in MintableIncentivizedERC721 contract:

    /**
     * @dev See {IERC721-safeTransferFrom}.
     */
    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId
    ) external virtual override nonReentrant {
        _safeTransferFrom(from, to, tokenId, "");
    }

    /**
     * @dev See {IERC721-safeTransferFrom}.
     */
    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        bytes memory _data
    ) external virtual override nonReentrant {
        _safeTransferFrom(from, to, tokenId, _data);
    }

    function _safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        bytes memory _data
    ) internal {
        require(
            _isApprovedOrOwner(_msgSender(), tokenId),
            "ERC721: transfer caller is not owner nor approved"
        );
        _safeTransfer(from, to, tokenId, _data);
    }

    /**
     * @dev Safely transfers `tokenId` token from `from` to `to`, checking first that contract recipients
     * are aware of the ERC721 protocol to prevent tokens from being forever locked.
     *
     * `_data` is additional data, it has no specified format and it is sent in call to `to`.
     *
     * This internal function is equivalent to {safeTransferFrom}, and can be used to e.g.
     * implement alternative mechanisms to perform token transfer, such as signature-based.
     *
     * Requirements:
     *
     * - `from` cannot be the zero address.
     * - `to` cannot be the zero address.
     * - `tokenId` token must exist and be owned by `from`.
     *
     * Emits a {Transfer} event.
     */
    function _safeTransfer(
        address from,
        address to,
        uint256 tokenId,
        bytes memory
    ) internal virtual {
        _transfer(from, to, tokenId);
    }

    /**
     * @dev Transfers `tokenId` from `from` to `to`.
     *  As opposed to {transferFrom}, this imposes no restrictions on msg.sender.
     *
     * Requirements:
     *
     * - `to` cannot be the zero address.
     * - `tokenId` token must be owned by `from`.
     *
     * Emits a {Transfer} event.
     */
    function _transfer(
        address from,
        address to,
        uint256 tokenId
    ) internal virtual {
        MintableERC721Logic.executeTransfer(
            _ERC721Data,
            POOL,
            ATOMIC_PRICING,
            from,
            to,
            tokenId
        );
    }

As you can see there is no check that if recipient address is contract and it's aware of receiving ERC721 token(there is no calling for on receive function) and the data parameter has been get ignored in the _safeTransfer() function. so this contract is not a valid implementation of the ERC721 token standard and because all the NToken* of protocol is child of this contract so they would not be a valid ERC712 contract too.

Tools Used

VIM

Recommended Mitigation Steps

properly implement ERC721 token

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/tokenization/libraries/ApeStakingLogic.sol#L31-L56

Vulnerability details

Impact

Function withdrawBAKC() should withdraw Ape coin staking position from ApeCoinStaking and sends unstaked apeCoin amount to user. but the code sends all the apeCoin balance of NToken contract to caller. so if contract had apeCoin balance it would send them to user too. there are multiple reason that contract could have apeCoin balance, it could be air dropped tokens or apeCoin can be underlying asset or staking reward of other users or apeCoin balance of other users in the contract during calls to other function which it's possible to perform reentrancy attack and take those balance by calling withdrawBAKC().

Proof of Concept

This is withdrawBAKC() code in ApeStakingLogic:

    /**
     * @notice withdraw Ape coin staking position from ApeCoinStaking
     * @param _apeCoinStaking ApeCoinStaking contract address
     * @param poolId identify whether BAYC or MAYC paired with BAKC
     * @param _nftPairs Array of Paired BAYC/MAYC NFT's with staked amounts
     * @param _apeRecipient the receiver of ape coin
     */
    function withdrawBAKC(
        ApeCoinStaking _apeCoinStaking,
        uint256 poolId,
        ApeCoinStaking.PairNftWithAmount[] memory _nftPairs,
        address _apeRecipient
    ) external {
        ApeCoinStaking.PairNftWithAmount[]
            memory _otherPairs = new ApeCoinStaking.PairNftWithAmount[](0);

        if (poolId == BAYC_POOL_ID) {
            _apeCoinStaking.withdrawBAKC(_nftPairs, _otherPairs);
        } else {
            _apeCoinStaking.withdrawBAKC(_otherPairs, _nftPairs);
        }

        uint256 balance = _apeCoinStaking.apeCoin().balanceOf(address(this));

        _apeCoinStaking.apeCoin().safeTransfer(_apeRecipient, balance);
    }

As you can see contract sends all the apeCoin balance of itself to recipient address but instead it should have only send the unstaked amount to user. If contract have some extra apeCoin as airDrop or other reasons then it will lose them. There are some scenarios that it's possible to perform reentrancy attack steal apeCoin balance of other users, attacker only needs to call withdrawBAKC() during other transactions and as there is no reentrancy protection for this contract it's possible to perform reentrancy. for example during the call to executeSetUnstakeApeIncentive(), attacker can be incentive and when contract trying to transfer incentive tokens from recived apeCoins attacker would call withdrawBAKC() and steal all the apeCoins.

Tools Used

VIM

Recommended Mitigation Steps

calculate unstaked amount by keeping balance of contract before and after the withdrawBAKC() call and those exact amount to user.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/tokenization/NTokenMoonBirds.sol#L63-L90

Vulnerability details

Impact

Contract NTokenMoonBirds can't receive airdropped tokens that are sent by safeTransferFrom() functions because function onERC721Received() only accepts calls from underlyingAsset. this would prevent users from receiving airdropped tokens for their staked tokens. for other NToken contract, function onERC721Received() won't reject calls and admin can later send users tokens with rescueERC721() function.

Proof of Concept

This is onERC721Received() code in NTokenMoonBirds contract:

    function onERC721Received(
        address operator,
        address from,
        uint256 id,
        bytes memory
    ) external virtual override returns (bytes4) {
        // only accept MoonBird tokens
        require(msg.sender == _underlyingAsset, Errors.OPERATION_NOT_SUPPORTED);

        // if the operator is the pool, this means that the pool is transferring the token to this contract
        // which can happen during a normal supplyERC721 pool tx
        if (operator == address(POOL)) {
            return this.onERC721Received.selector;
        }

        // supply the received token to the pool and set it as collateral
        DataTypes.ERC721SupplyParams[]
            memory tokenData = new DataTypes.ERC721SupplyParams[](1);

        tokenData[0] = DataTypes.ERC721SupplyParams({
            tokenId: id,
            useAsCollateral: true
        });

        POOL.supplyERC721FromNToken(_underlyingAsset, tokenData, from);

        return this.onERC721Received.selector;
    }

As you can see it only accepts calls from underlyingAsset and if another address send tokens by calling function safeTransferFrom() of that token, the call would revert, because token would call onERC721Received() to check that target address is accepting ERC721 tokens or not and NTokenMoonBirds would reject it so NTokenMoonBirds can't receive those tokens. for airdropped tokens which get transferred by safeTransferFrom() or safeTransfer() functions of airdrop token, this bug would happen and NTokenMoonBirds would reject those airdrops and users who deposit their tokens as collateral won't receive their airdrops. other NTokens accept receiving tokens in function onERC721Received() and later admin can distribute those airdrops with rescueERC721().

Tools Used

VIM

Recommended Mitigation Steps

function onERC721Received() shouldn't reject other tokens and it should accept receiving other tokens and only call pool.supplyERC721FromNToken() when msg.sender == _underlyingAsset. so the require statement should be changed to simple if statement.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/pool/DefaultReserveAuctionStrategy.sol#L90-L135 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/tokenization/libraries/MintableERC721Logic.sol#L424-L434

Vulnerability details

Impact

attacker can liquidate users NFT collaterals with min price immediately after user health factor gets below the liquidation threshold for NFT collaterals that have old open auctions and setAuctionValidityTime() is not get called for the user. whenever user's account health factor gets below the NFT liquidation threshold attacker can start auction for all of users NFTs in the protocol. user need to call setAuctionValidityTime() to invalidated all of those open auctions whenever his/her account health factor is good, but if user don't call this and don't close those auctions then next time user's accounts health factor gets below the threshold attacker would liquidate all of those NFTs with minimum price immediately.

Proof of Concept

This is calculateAuctionPriceMultiplier() code in DefaultReserveAuctionStrategy:

    function calculateAuctionPriceMultiplier(
        uint256 auctionStartTimestamp,
        uint256 currentTimestamp
    ) external view override returns (uint256) {
        uint256 ticks = PRBMathUD60x18.div(
            currentTimestamp - auctionStartTimestamp,
            _tickLength
        );
        return _calculateAuctionPriceMultiplierByTicks(ticks);
    }

    function _calculateAuctionPriceMultiplierByTicks(uint256 ticks)
        internal
        view
        returns (uint256)
    {
        if (ticks < PRBMath.SCALE) {
            return _maxPriceMultiplier;
        }

        uint256 ticksMinExp = PRBMathUD60x18.div(
            (PRBMathUD60x18.ln(_maxPriceMultiplier) -
                PRBMathUD60x18.ln(_minExpPriceMultiplier)),
            _stepExp
        );
        if (ticks <= ticksMinExp) {
            return
                PRBMathUD60x18.div(
                    _maxPriceMultiplier,
                    PRBMathUD60x18.exp(_stepExp.mul(ticks))
                );
        }

        uint256 priceMinExpEffective = PRBMathUD60x18.div(
            _maxPriceMultiplier,
            PRBMathUD60x18.exp(_stepExp.mul(ticksMinExp))
        );
        uint256 ticksMin = ticksMinExp +
            (priceMinExpEffective - _minPriceMultiplier).div(_stepLinear);

        if (ticks <= ticksMin) {
            return priceMinExpEffective - _stepLinear.mul(ticks - ticksMinExp);
        }

        return _minPriceMultiplier;
    }

As you can see when long times passed from auction start time the price of auction would be minimum. This is isAuctioned() code in MintableERC721Data contract:

    function isAuctioned(
        MintableERC721Data storage erc721Data,
        IPool POOL,
        uint256 tokenId
    ) public view returns (bool) {
        return
            erc721Data.auctions[tokenId].startTime >
            POOL
                .getUserConfiguration(erc721Data.owners[tokenId])
                .auctionValidityTime;
    }

As you can see auction is valid if startTime is bigger than user's auctionValidityTime. but the value of auctionValidityTime should be set manually by calling PoolParameters.setAuctionValidityTime(), so by default auction would stay open. imagine this scenario:

1. user NFT health factor gets under the liquidation threshold.
2. attacker calls startAuction() for all user collateral NFT tokens.
3. user supply some collateral and his health factor become good.
4. some time passes and user NFT health factor gets under the liquidation threshold.
5. attacker can liquidate all of user NFT collateral with minimum price of auction immediately.
6. user would lose his NFTs without proper auction.

This scenario can be common because liquidation and health factor changes happens on-chain and most of the user isn't always watches his account health factor and he wouldn't know when his account health factor gets below the liquidation threshold and when it's needed for him to call setAuctionValidityTime(). so over time this would happen to more users.

Tools Used

VIM

Recommended Mitigation Steps

contract should check and set the value of auctionValidityTime for user whenever an action happens to user account. also there should be some incentive mechanism for anyone starting or ending an auction.

[[1]] in function executeSetIsUsedAsCollateral() of MintableERC721Logic owner check should be in the beginning of the function.

    function executeSetIsUsedAsCollateral(
        MintableERC721Data storage erc721Data,
        IPool POOL,
        uint256 tokenId,
        bool useAsCollateral,
        address sender
    ) internal returns (bool) {
        if (erc721Data.isUsedAsCollateral[tokenId] == useAsCollateral)
            return false;

        address owner = erc721Data.owners[tokenId];
        require(owner == sender, "not owner");

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/tokenization/libraries/MintableERC721Logic.sol#L153-L171

[[2]] in function executeStartAuction() and executeEndAuction() of MintableERC721Logic code should check the existence before checking auction status.

    function executeStartAuction(
        MintableERC721Data storage erc721Data,
        IPool POOL,
        uint256 tokenId
    ) external {
        require(
            !isAuctioned(erc721Data, POOL, tokenId),
            Errors.AUCTION_ALREADY_STARTED
        );
        require(
            _exists(erc721Data, tokenId),
            "ERC721: startAuction for nonexistent token"
        );
        erc721Data.auctions[tokenId] = DataTypes.Auction({
            startTime: block.timestamp
        });
    }

    function executeEndAuction(
        MintableERC721Data storage erc721Data,
        IPool POOL,
        uint256 tokenId
    ) external {
        require(
            isAuctioned(erc721Data, POOL, tokenId),
            Errors.AUCTION_NOT_STARTED
        );
        require(
            _exists(erc721Data, tokenId),
            "ERC721: endAuction for nonexistent token"
        );
        delete erc721Data.auctions[tokenId];
    }

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/tokenization/libraries/MintableERC721Logic.sol#L386-L400 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/tokenization/libraries/MintableERC721Logic.sol#L368-L384

[[3]] in functions withdrawBAKC() and withdrawApeCoin() of PoolApeStaking unused variable amountToWithdraw https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/pool/PoolApeStaking.sol#L120-L185 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/pool/PoolApeStaking.sol#L59-L90

[[4]] in function executeDropReserve() of PoolLogic code doesn't remove the item from list and as times passes reservesList[] length would grow and more gas would payed and if the length was too high the protocol would be broken. because looping through reservesList is used whenever HF is calculated which is in every call so the gas impact would be huge.

    /**
     * @notice Drop a reserve
     * @param reservesData The state of all the reserves
     * @param reservesList The addresses of all the active reserves
     * @param asset The address of the underlying asset of the reserve
     **/
    function executeDropReserve(
        mapping(address => DataTypes.ReserveData) storage reservesData,
        mapping(uint256 => address) storage reservesList,
        address asset
    ) external {
        DataTypes.ReserveData storage reserve = reservesData[asset];
        ValidationLogic.validateDropReserve(reservesList, reserve, asset);
        reservesList[reservesData[asset].id] = address(0);
        delete reservesData[asset];
    }

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/PoolLogic.sol#L138-L153

[[5]] The function initialize() is public and can be called by anyone as long as the contract is deployed. attacker can take ownership of implementation contract and harm the protocol. https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/pool/PoolCore.sol#L68-L88