ParaSpace contest - cccz's results

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/ui/WPunkGateway.sol#L77-L83

Vulnerability details

Impact

When Punk.buyPunk is called in WPunkGateway, no ETH is sent, so the user can only buy free punk through Punk.buyPunk when calling WPunkGateway.providePunk/acceptBidWithCredit/batchAcceptBidWithCredit.

    function buyPunk(uint punkIndex) payable {
        if (!allPunksAssigned) throw;
        Offer offer = punksOfferedForSale[punkIndex];
        if (punkIndex >= 10000) throw;
        if (!offer.isForSale) throw;                // punk not actually for sale
        if (offer.onlySellTo != 0x0 && offer.onlySellTo != msg.sender) throw;  // punk not supposed to be sold to this user
        if (msg.value < offer.minValue) throw;      // Didn't send enough ETH
        if (offer.seller != punkIndexToAddress[punkIndex]) throw; // Seller no longer owner of punk

As a user, you can only consider buying punk first, then call Punk.offerPunkForSaleToAddress or Punk.offerPunkForSale to create a free sale (where offerPunkForSaleToAddress requires WPunkGateway to be set to toAddress), then call WPunkGateway.providePunk/acceptBidWithCredit/batchAcceptBidWithCredit.

    function offerPunkForSale(uint punkIndex, uint minSalePriceInWei) {
        if (!allPunksAssigned) throw;
        if (punkIndexToAddress[punkIndex] != msg.sender) throw;
        if (punkIndex >= 10000) throw;
        punksOfferedForSale[punkIndex] = Offer(true, punkIndex, msg.sender, minSalePriceInWei, 0x0);
        PunkOffered(punkIndex, minSalePriceInWei, 0x0);
    }

    function offerPunkForSaleToAddress(uint punkIndex, uint minSalePriceInWei, address toAddress) {
        if (!allPunksAssigned) throw;
        if (punkIndexToAddress[punkIndex] != msg.sender) throw;
        if (punkIndex >= 10000) throw;
        punksOfferedForSale[punkIndex] = Offer(true, punkIndex, msg.sender, minSalePriceInWei, toAddress);
        PunkOffered(punkIndex, minSalePriceInWei, toAddress);
    }

This allows malicious users to front-run WPunkGateway.supplyPunk/acceptBidWithCredit/batchAcceptBidWithCredit to steal punk through these free sales.

Proof of Concept

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/ui/WPunkGateway.sol#L77-L83 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/ui/WPunkGateway.sol#L129-L137 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/ui/WPunkGateway.sol#L167-L175

Tools Used

None

Recommended Mitigation Steps

Consider adding the payable attribute to WPunkGateway.providePunk/acceptBidWithCredit/batchAcceptBidWithCredit and sending ETH when calling Punk.buyPunk in WPunkGateway to allow users to buy punk directly from the sale.

    function supplyPunk(
        DataTypes.ERC721SupplyParams[] calldata punkIndexes,
        address onBehalfOf,
        uint16 referralCode
-    ) external nonReentrant {
+    ) external payable nonReentrant {
        for (uint256 i = 0; i < punkIndexes.length; i++) {
-            Punk.buyPunk(punkIndexes[i].tokenId);
+            Punk.buyPunk{value:msg.value}(punkIndexes[i].tokenId);

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/ReserveLogic.sol#L169-L186

Vulnerability details

Impact

The ReserveLogic.updateInterestRates function calls DefaultReserveInterestRateStrategy.calculateInterestRates to calculate the new interest rate

    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(

The calculateInterestRates function calculates the interest rate based on the current liquidity in the PToken, where liquidityAdded and liquidityTaken indicate the next liquidity to be added or taken from the PToken, and they are added to the calculation to ensure that the latest liquidity is used.

    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;

For example, in supply, the correct process is (I consulted with aave members)

cache
updateState
validation
updateInterestRates (x amount as liquidityAdded, balanceOf() does not account of the future transfer)
transfer x amount of assets to the PToken
...
    function executeSupply(
        mapping(address => DataTypes.ReserveData) storage reservesData,
        DataTypes.UserConfigurationMap storage userConfig,
        DataTypes.ExecuteSupplyParams memory params
    ) external {
        DataTypes.ReserveData storage reserve = reservesData[params.asset];
        DataTypes.ReserveCache memory reserveCache = reserve.cache();

        reserve.updateState(reserveCache);

        ValidationLogic.validateSupply(
            reserveCache,
            params.amount,
            DataTypes.AssetType.ERC20
        );

        reserve.updateInterestRates(
            reserveCache,
            params.asset,
            params.amount,
            0
        );

        IERC20(params.asset).safeTransferFrom(
            params.payer,
            reserveCache.xTokenAddress,
            params.amount
        );

But in LiquidationLogic._burnDebtTokens, for liquidationAsset, the process is

cache
updateState
validation
transfer x amount of assets to the PToken
updateInterestRates (x amount as liquidityAdded, but balanceOf() already account of the transfer)
...
    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
        );
    }

This will cause the liquidity to be exaggerated, resulting in the calculated interest rate being smaller, thus reducing the liquidity provider's reward and the borrower's debt

Proof of Concept

Consider that the current liquidationAsset has a debt rate of 0.8%, and when the user liquidates, the debt rate should decrease to 0.7% because new liquidationAsset is added to the liquidity, but when the rate is calculated in the _burnDebtTokens function, the increased liquidity is exaggerated by a factor of two (because the actualLiquidationAmount is double-counted in the liquidity), resulting in a lower debt rate of 0.6%, and the user borrows at this point, then until the next updateInterestRates is called, he will enjoys a lower debt rate. And, if liquidation occurs frequently enough, the debt rate and liquidity rate will be severely underestimated.

@audit indicates how liquidity affects interest rates

    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 =  // @audit: bigger
                IToken(params.reserve).balanceOf(params.xToken) +
                params.liquidityAdded -
                params.liquidityTaken;

            vars.availableLiquidityPlusDebt = // @audit: bigger 
                vars.availableLiquidity +
                vars.totalDebt;
            vars.borrowUsageRatio = vars.totalDebt.rayDiv(    // @audit: smaller
                vars.availableLiquidityPlusDebt 
            );
            vars.supplyUsageRatio = vars.totalDebt.rayDiv(    // @audit: smaller
                vars.availableLiquidityPlusDebt 
            );
        }

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

            vars.currentVariableBorrowRate +=  //@audit: smaller
                _variableRateSlope1 +
                _variableRateSlope2.rayMul(excessBorrowUsageRatio);
        } else {
            vars.currentVariableBorrowRate += _variableRateSlope1 //@audit: smaller
                .rayMul(vars.borrowUsageRatio)
                .rayDiv(OPTIMAL_USAGE_RATIO); 
        }

        vars.currentLiquidityRate = vars   // @audit: smaller
            .currentVariableBorrowRate
            .rayMul(vars.supplyUsageRatio)
            .percentMul(
                PercentageMath.PERCENTAGE_FACTOR - params.reserveFactor
            );

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

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/ReserveLogic.sol#L169-L186 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/pool/DefaultReserveInterestRateStrategy.sol#L127-L141

Tools Used

None

Recommended Mitigation Steps

In _burnDebtTokens, call updateInterestRates before transferring liquidationAsset

    function _burnDebtTokens(
        DataTypes.ReserveData storage liquidationAssetReserve,
        DataTypes.ExecuteLiquidateParams memory params,
        ExecuteLiquidateLocalVars memory vars
    ) internal {
        _depositETH(params, vars);
+        // Update borrow & supply rate
+        liquidationAssetReserve.updateInterestRates(
+            vars.liquidationAssetReserveCache,
+            params.liquidationAsset,
+            vars.actualLiquidationAmount,
+            0
+        );
        // 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
-        );
    }

Judge has assessed an item in Issue #72 as M risk. The relevant finding follows:

[Low-03] NTokenMoonBirds may not be able to receive airdrops Impact For most NToken, some airdrops that are actively minted to the holder's address can be withdrawn and later distributed by the PoolAdmin calling the rescueERC721 function.

function rescueERC721(
    address token,
    address to,
    uint256[] calldata ids
) external override onlyPoolAdmin {
    require(
        token != _underlyingAsset,
        Errors.UNDERLYING_ASSET_CAN_NOT_BE_TRANSFERRED
    );
    for (uint256 i = 0; i < ids.length; i++) {
        IERC721(token).safeTransferFrom(address(this), to, ids[i]);
    }
    emit RescueERC721(token, to, ids);
}

However, in the onERC721Received function of the NTokenMoonBirds contract, due to the requirement that the sender can only be the MoonBird contract, when safemint()/safetransferfrom() is called to send the airdrop NFTs to the NTokenMoonBirds contract, the transaction will fail, thus preventing NTokenMoonBirds from receiving these airdrops.

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);

For example, Moonbirds Oddities are actively minted to the holder's address. https://etherscan.io/tx/0x3af5de8b6a8c55aac033d57e1b110e8340abf4dcd289ebda889a44f9f9dc613d

Proof of Concept https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/tokenization/NToken.sol#L136-L149 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/tokenization/NTokenMoonBirds.sol#L63-L77

Recommended Mitigation Steps Consider allowing the NTokenMoonBirds contract to receive NFTs from other addresses and only call POOL.supportERC721FromNToken when msg.sender == _underlyingAsset

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;
    }
  

• if(msg.sender == _underlyingAsset){       
    // 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; }

[Low-01] ValidationLogic.validateLiquidateERC721: msg.value should be greater than actualLiquidationAmount not maxLiquidationAmount

Impact

In validateLiquidateERC721, maxLiquidationAmount is the maximum amount of expenditure entered by the user, and actualLiquidationAmount is the maximum liquidation amount that the user can liquidate. When the asset is ETH, msg.value should be greater than actualLiquidationAmount, which is consistent with validateLiquidateERC20

         require(
             msg.value == 0 || msg.value >= params.actualLiquidationAmount,
             Errors.LIQUIDATION_AMOUNT_NOT_ENOUGH
         );

Proof of Concept

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/ValidationLogic.sol#L672-L676

Recommended Mitigation Steps

        require(
            params.maxLiquidationAmount >= params.actualLiquidationAmount &&
-               (msg.value == 0 || msg.value >= params.maxLiquidationAmount),
+               (msg.value == 0 || msg.value >= params.actualLiquidationAmount),
            Errors.LIQUIDATION_AMOUNT_NOT_ENOUGH
        );

[Low-02] Implementation does not match documentation

Impact

https://parallelfinance.notion.site/Audit-Technical-Documentation-0a107270dabe45d2b66a076e0bdaa943 The docs say

During ERC721 liquidation, we allow the liquidationAsset to be any ERC20 (not only the debtAsset of borrower), if the liquidation asset is not debt asset then basically there is no liquidation bonus. 

But in fact, only eth/weth is allowed to liquidate ERC721, and there is no cancellation of liquidation rewards by comparing liquidation assets and debt assets

Proof of Concept

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/pool/PoolCore.sol#L477-L478

Recommended Mitigation Steps

Change the documentation description, or change the implementation

[Low-03] NTokenMoonBirds may not be able to receive airdrops

Impact

For most NToken, some airdrops that are actively minted to the holder's address can be withdrawn and later distributed by the PoolAdmin calling the rescueERC721 function.

    function rescueERC721(
        address token,
        address to,
        uint256[] calldata ids
    ) external override onlyPoolAdmin {
        require(
            token != _underlyingAsset,
            Errors.UNDERLYING_ASSET_CAN_NOT_BE_TRANSFERRED
        );
        for (uint256 i = 0; i < ids.length; i++) {
            IERC721(token).safeTransferFrom(address(this), to, ids[i]);
        }
        emit RescueERC721(token, to, ids);
    }

However, in the onERC721Received function of the NTokenMoonBirds contract, due to the requirement that the sender can only be the MoonBird contract, when safemint()/safetransferfrom() is called to send the airdrop NFTs to the NTokenMoonBirds contract, the transaction will fail, thus preventing NTokenMoonBirds from receiving these airdrops.

    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);

For example, Moonbirds Oddities are actively minted to the holder's address. https://etherscan.io/tx/0x3af5de8b6a8c55aac033d57e1b110e8340abf4dcd289ebda889a44f9f9dc613d

Proof of Concept

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

Recommended Mitigation Steps

Consider allowing the NTokenMoonBirds contract to receive NFTs from other addresses and only call POOL.supportERC721FromNToken when msg.sender == _underlyingAsset

    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;
        }
+     if(msg.sender == _underlyingAsset){       
        // 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;
    }