ParaSpace contest - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

While removing any feeder the index is not updated. This incorrection in indexing will lead to _removeFeeder fail everytime as shown in POC. This could lead to problem when owner wants to remove a malicious feeder and would be unable to.

Proof of Concept

1. Lets say owner has added 2 feeders as shown below:

feeders = {A,B}
feederPositionMap[A].index = 0
feederPositionMap[B].index = 1

2. Now owner removes feeder A by calling the removeFeeder function

function removeFeeder(address _feeder)
        external
        onlyWhenFeederExisted(_feeder)
    {
        _removeFeeder(_feeder);
    }

3. This internally calls _removeFeeder function

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

4. In our case below is state of variables:

uint8 feederIndex = feederPositionMap[_feeder].index; = feederPositionMap[A].index = 0

if (feederIndex >= 0 && feeders[feederIndex] == _feeder) { // true since feederIndex is 0
feeders[feederIndex] = feeders[feeders.length - 1]; = feeders[2-1] = feeders[1] = B // thus feeders[0] = B
  feeders.pop(); // This deletes the last feeder, so feeders becomes {B}
}

delete feederPositionMap[_feeder];  // delete feederPositionMap[A];
        revokeRole(UPDATER_ROLE, _feeder);
        emit FeederRemoved(_feeder);

5. As shown in previous step, last feeder is removed and removed index is updated with last feeder. This makes:

feeders = {B}
feederPositionMap[B].index = 1 // since this was never updated

6. Owner decides to remove feeder B. Below is the code flow

uint8 feederIndex = feederPositionMap[_feeder].index;  = feederPositionMap[B].index; = 1
        if (feederIndex >= 0 && feeders[feederIndex] == _feeder) { // fails due to overflow, feeders length is 1 so max index is 0
            ...
        }

7. As we can see this fails due to array index overflow since it tries to delete feeder from index 1 even though after deletion of A, feeder max index could be 0

Recommended Mitigation Steps

Update the index as shown below:

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

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/main/paraspace-core/contracts/protocol/libraries/logic/LiquidationLogic.sol#L531

Vulnerability details

Impact

The debt tokens are being transferred before calculating the interest rates. But the interest rate calculation function assumes that debt token has not yet been sent thus the outcome currentLiquidityRate will be incorrect

Proof of Concept

1. Liquidator L1 calls executeLiquidateERC20 for a position whose health factor <1

function executeLiquidateERC20(
        mapping(address => DataTypes.ReserveData) storage reservesData,
        mapping(uint256 => address) storage reservesList,
        mapping(address => DataTypes.UserConfigurationMap) storage usersConfig,
        DataTypes.ExecuteLiquidateParams memory params
    ) external returns (uint256) {

...
 _burnDebtTokens(liquidationAssetReserve, params, vars);
...
}

2. This internally calls _burnDebtTokens

    function _burnDebtTokens(
        DataTypes.ReserveData storage liquidationAssetReserve,
        DataTypes.ExecuteLiquidateParams memory params,
        ExecuteLiquidateLocalVars memory vars
    ) internal {
       ...

        // 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
        );
...
        // Update borrow & supply rate
        liquidationAssetReserve.updateInterestRates(
            vars.liquidationAssetReserveCache,
            params.liquidationAsset,
            vars.actualLiquidationAmount,
            0
        );
    }

3. Basically first it transfers the debt asset to xToken using below. This increases the balance of xTokenAddress for liquidationAsset

IERC20(params.liquidationAsset).safeTransferFrom(
            vars.payer,
            vars.liquidationAssetReserveCache.xTokenAddress,
            vars.actualLiquidationAmount
        );

4. Now updateInterestRates function is called on ReserveLogic.sol#L169

function updateInterestRates(
        DataTypes.ReserveData storage reserve,
        DataTypes.ReserveCache memory reserveCache,
        address reserveAddress,
        uint256 liquidityAdded,
        uint256 liquidityTaken
    ) internal {
...
(
            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
                })
            );
...
}

5. Finally call to calculateInterestRates function on DefaultReserveInterestRateStrategy#L127 contract is made which calculates the interest rate

function calculateInterestRates(
        DataTypes.CalculateInterestRatesParams calldata params
    ) external view override returns (uint256, uint256) {
...
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
            );
        }
...
vars.currentLiquidityRate = vars
            .currentVariableBorrowRate
            .rayMul(vars.supplyUsageRatio)
            .percentMul(
                PercentageMath.PERCENTAGE_FACTOR - params.reserveFactor
            );

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

6. As we can see in above code, vars.availableLiquidity is calculated as IToken(params.reserve).balanceOf(params.xToken) +params.liquidityAdded - params.liquidityTaken

7. But the problem is that debt token is already transferred to xToken which means xToken already consist of params.liquidityAdded. Hence the calculation ultimately becomes (xTokenBeforeBalance+params.liquidityAdded) +params.liquidityAdded - params.liquidityTaken

8. This is incorrect and would lead to higher vars.availableLiquidity which ultimately impacts the currentLiquidityRate

Recommended Mitigation Steps

Transfer the debt asset post interest calculation

function _burnDebtTokens(
        DataTypes.ReserveData storage liquidationAssetReserve,
        DataTypes.ExecuteLiquidateParams memory params,
        ExecuteLiquidateLocalVars memory vars
    ) internal {
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
            );

liquidationAssetReserve.updateInterestRates(
            vars.liquidationAssetReserveCache,
            params.liquidationAsset,
            vars.actualLiquidationAmount,
            0
        );
IERC20(params.liquidationAsset).safeTransferFrom(
            vars.payer,
            vars.liquidationAssetReserveCache.xTokenAddress,
            vars.actualLiquidationAmount
        );
...
...
}

Lines of code

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

Vulnerability details

Impact

It was observed that removeFeeder can be called by any user as it is missing any ACL. This means any user could remove all the feeders and hence impact the oracle pricing system (Feeders wont be able to add prices if removed)

Proof of Concept

1. Observe the removeFeeder function

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

2. Notice as per comment only owner is allowed to remove the feed but there is no modifier to verify the same, meaning any user can call this function to remove any feeders

Recommended Mitigation Steps

Add a modifier to check that caller is owner (Notice onlyWhenFeederExisted(_feeder) is not required since already called in _removeFeeder function)

function removeFeeder(address _feeder)
        external
        onlyRole(DEFAULT_ADMIN_ROLE)
    {
        _removeFeeder(_feeder);
    }

Lines of code

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

Vulnerability details

Impact

The safeTransfer function 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. But seems like this safety check got missed in the _safeTransfer function leading to non secure ERC721 transfers

Proof of Concept

1. User calls the safeTransferFrom function (Using NToken contract which implements MintableIncentivizedERC721 contract)

function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        bytes memory _data
    ) external virtual override nonReentrant {
        _safeTransferFrom(from, to, tokenId, _data);
    }

2. This makes an internal call to _safeTransferFrom -> _safeTransfer -> _transfer

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

function _safeTransfer(
        address from,
        address to,
        uint256 tokenId,
        bytes memory
    ) internal virtual {
        _transfer(from, to, tokenId);
    }

3. Now lets see _transfer function

function _transfer(
        address from,
        address to,
        uint256 tokenId
    ) internal virtual {
        MintableERC721Logic.executeTransfer(
            _ERC721Data,
            POOL,
            ATOMIC_PRICING,
            from,
            to,
            tokenId
        );
    }

4. This is calling MintableERC721Logic.executeTransfer which simply transfers the asset

5. In this full flow there is no check to see whether to address can support ERC721 which fails the purpose of safeTransferFrom function

6. Also notice the comment mentions that data parameter passed in safeTransferFrom is sent to recipient in call but there is no such transfer of data

Recommended Mitigation Steps

Add a call to onERC721Received for recipient and see if the recipient actually supports ERC721

Lines of code

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

Vulnerability details

Impact

The ApeStakingLogic library is not handling the withdraw correctly and will transfer user any existing balance held by the contract in addition to user requested amount.

Proof of Concept

1. Assume initial NToken ApeCoin balance is 1000 at NTokenMAYC contract (Also true for NTokenBAYC)

2. User call withdrawBAKC function at PoolApeStaking#L120

function withdrawBAKC(
        address nftAsset,
        ApeCoinStaking.PairNftWithAmount[] memory _nftPairs
    ) external nonReentrant {
	...
	nTokenApeStaking.withdrawBAKC(_nftPairs, msg.sender);
	...
	}

3. This makes withdrawBAKC function call to NTokenMAYC contract (Assume passed Paired argument contained MAYC NFT's)

function withdrawBAKC(
        ApeCoinStaking.PairNftWithAmount[] memory _nftPairs,
        address _apeRecipient
    ) external onlyPool nonReentrant {
        ApeStakingLogic.withdrawBAKC(
            _apeCoinStaking,
            POOL_ID(),
            _nftPairs,
            _apeRecipient
        );
    }

4. This uses library ApeStakingLogic function withdrawBAKC which is implemented as below:

function withdrawBAKC(
        ApeCoinStaking _apeCoinStaking,
        uint256 poolId,
        ApeCoinStaking.PairNftWithAmount[] memory _nftPairs,
        address _apeRecipient
    ) external {
        ...

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

5. This calls withdrawBAKC on the contract ApeCoinStaking

function withdrawBAKC(PairNftWithAmount[] calldata _baycPairs, PairNftWithAmount[] calldata _maycPairs) external {
        updatePool(BAKC_POOL_ID);
        _withdrawPairNft(BAYC_POOL_ID, _baycPairs);
        _withdrawPairNft(MAYC_POOL_ID, _maycPairs);
    }

function _withdrawPairNft(uint256 mainTypePoolId, PairNftWithAmount[] calldata _nfts) private {
...
_withdraw(BAKC_POOL_ID, position, amount, mainTokenOwner);
...
}

function _withdraw(uint256 _poolId, Position storage _position, uint256 _amount, address _recipient) private {
        ...

        apeCoin.safeTransfer(_recipient, _amount);
    }

6. NTokenMAYC contract receives the withdrawn apeCoin say X amount. The final balance of contract becomes now 1000+X

7. Finally the last logic at withdrawBAKC function at ApeStakingLogic executes

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

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

8. Since this is transferring all ape coin balance so full 1000+X balance is transferred to User even though only X amount should be transferred

Recommended Mitigation Steps

Keep hold of initial apeCoin balance before calling _apeCoinStaking.withdrawBAKC. Subtract the final balance with this initial balance to get the correct amount to send

function withdrawBAKC(
        ApeCoinStaking _apeCoinStaking,
        uint256 poolId,
        ApeCoinStaking.PairNftWithAmount[] memory _nftPairs,
        address _apeRecipient
    ) external {
        ApeCoinStaking.PairNftWithAmount[]
            memory _otherPairs = new ApeCoinStaking.PairNftWithAmount[](0);
uint256 initialBalance = _apeCoinStaking.apeCoin().balanceOf(address(this));
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-initialBalance);
}

Lines of code

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

Vulnerability details

Impact

The current implementation of NTokenMoonBirds restricts it to receive any airdrop. This is because onERC721Received only allows MoonBird contract as sender of NFT which means any other sender is plainly rejected

Proof of Concept

1. Airdrop NFT is sent to NTokenMoonBirds contract using say safeTransferFrom function
2. Transfer function invokes the onERC721Received method on receiving contract which in this case is NTokenMoonBirds contract
3. Airdrop gets rejected since sender is not MoonDrop 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);
...
}

Recommended Mitigation Steps

The hardrcore requirement for sender can be removed and contract should allow airdrop NFT

function onERC721Received(
        address operator,
        address from,
        uint256 id,
        bytes memory
    ) external virtual override returns (bytes4) {

        // 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;
        }
// only accept MoonBird tokens
        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;
    }

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

Support for IERC165 interface id is missed Contract: https://github.com/code-423n4/2022-11-paraspace/blob/main/paraspace-core/contracts/protocol/tokenization/base/MintableIncentivizedERC721.sol#L572

Impact: Contract fails to support a valid interface which could lead to failure of genuine calls

Steps:

Observe the supportsInterface function supportsInterface(bytes4 interfaceId) external view virtual override(IERC165) returns (bool) { return interfaceId == type(IERC721Enumerable).interfaceId || interfaceId == type(IERC721Metadata).interfaceId; } Observe that support for IERC165 interface id is missing Recommendation: Kindly revise the function as below:

function supportsInterface(bytes4 interfaceId) external view virtual override(IERC165) returns (bool) { return interfaceId == type(IERC721Enumerable).interfaceId || interfaceId == type(IERC721Metadata).interfaceId || interfaceId == type(IERC165).interfaceId; }

Valid Approved users will not be allowed to transfer

Contract: https://github.com/code-423n4/2022-11-paraspace/blob/main/paraspace-core/contracts/protocol/tokenization/base/MintableIncentivizedERC721.sol#L412

Impact: The transferFrom allows an approved user to transfer NFT on behalf of owner. But seems like this feature is broken since one of its internal call requires caller to be owner only which means the call would fail

Steps:

1. Approved User calls the transferFrom function

function transferFrom(
        address from,
        address to,
        uint256 tokenId
    ) external virtual override nonReentrant {
        //solhint-disable-next-line max-line-length
        require(
            _isApprovedOrOwner(_msgSender(), tokenId),
            "ERC721: transfer caller is not owner nor approved"
        );

        _transfer(from, to, tokenId);
    }

2. This makes call to _transfer function. Since this is an abstract contract which is implemented by NToken contract. Now NToken has overridden its own _transfer function

function _transfer(
        address from,
        address to,
        uint256 tokenId,
        bool validate
    ) internal virtual {
        address underlyingAsset = _underlyingAsset;

        uint256 fromBalanceBefore = collateralizedBalanceOf(from);
        uint256 toBalanceBefore = collateralizedBalanceOf(to);
        bool isUsedAsCollateral = _transferCollateralizable(from, to, tokenId);
...
}

3. This calls _transferCollateralizable which internally calls executeTransfer function

unction _transferCollateralizable(
        address from,
        address to,
        uint256 tokenId
    ) internal virtual returns (bool isUsedAsCollateral_) {
        isUsedAsCollateral_ = MintableERC721Logic
            .executeTransferCollateralizable(
                _ERC721Data,
                POOL,
                ATOMIC_PRICING,
                from,
                to,
                tokenId
            );
    }

function executeTransferCollateralizable(
        ...
    ) external returns (bool isUsedAsCollateral_) {
        ...

        executeTransfer(erc721Data, POOL, ATOMIC_PRICING, from, to, tokenId);
    }

4. The executeTransfer function checks whether caller is owner itself. Since caller is an approved user and not owner so this call fails

function executeTransfer(
        MintableERC721Data storage erc721Data,
        IPool POOL,
        bool ATOMIC_PRICING,
        address from,
        address to,
        uint256 tokenId
    ) public {
        require(
            erc721Data.owners[tokenId] == from,
            "ERC721: transfer from incorrect owner"
        );
...
}

Recommendation: Either remove the owner requirement or b) if this is meant to be called only by owner then remove the approved user allow

Centralization Risk

Contract: https://github.com/code-423n4/2022-11-paraspace/blob/main/paraspace-core/contracts/protocol/tokenization/base/MintableIncentivizedERC721.sol#L131

Impact: It seems the poolAdmin holds too much power including changing reward controller, rescue tokens etc. This can allow poolAdmin to impact all users by changing the config or draining the contract. In this example we will see one example for setIncentivesController

Steps:

1. PoolAdmin calls setIncentivesController and set rewardController to zero
2. This causes Users will stop getting incentives on their stakes. So if User decides to burn then the reward incentives are gone permanently

Recommendation: Keep the poolAdmin as multiSig and behind timelock to prevent immediate changes

Asset is not removed properly

Contract: https://github.com/code-423n4/2022-11-paraspace/blob/main/paraspace-core/contracts/misc/NFTFloorOracle.sol#L296

Impact: Asset are deleted directly leaving blanks in middle of assets. This could have unintended effects based on how assets is being utlized

Steps:

1. Assume there were 2 assets {A,B}
2. removeAsset function is called to remove asset A
3. This internally calls _removeAsset function

function _removeAsset(address _asset)
        internal
        onlyWhenAssetExisted(_asset)
    {
        uint8 assetIndex = assetFeederMap[_asset].index; = assetFeederMap[A].index = 0
        delete assets[assetIndex]; = delete assets[0];
        delete assetPriceMap[_asset];
        delete assetFeederMap[_asset];
        emit AssetRemoved(_asset);
    }

4. Since assets[0] is directly deleted and not swapped with last array value so assets would look like below:

assets[0] = {}
assets[1] = {B}

Recommendation: Instead of directly deleting the asset, swap deleted asset index with last index asset and then pop last element. Sample example is _removeFeeder

Support for IERC165 interface id is missed

Contract: https://github.com/code-423n4/2022-11-paraspace/blob/main/paraspace-core/contracts/protocol/tokenization/base/MintableIncentivizedERC721.sol#L572

Impact: Contract fails to support a valid interface which could lead to failure of genuine calls

Steps:

1. Observe the supportsInterface

function supportsInterface(bytes4 interfaceId)
        external
        view
        virtual
        override(IERC165)
        returns (bool)
    {
        return
            interfaceId == type(IERC721Enumerable).interfaceId ||
            interfaceId == type(IERC721Metadata).interfaceId;
    }

2. Observe that support for IERC165 interface id is missing

Recommendation: Kindly revise the function as below:

function supportsInterface(bytes4 interfaceId)
        external
        view
        virtual
        override(IERC165)
        returns (bool)
    {
        return
            interfaceId == type(IERC721Enumerable).interfaceId ||
            interfaceId == type(IERC721Metadata).interfaceId ||
            interfaceId == type(IERC165).interfaceId;
    }