ParaSpace contest - gzeon's results

Lines of code

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

Vulnerability details

Impact

When adding a asset to NFTFloorOracle, it set its index to assetFeederMap for easier lookup.

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

        assetFeederMap[_asset].index = uint8(assets.length - 1);

The problem is that the cast to uint8 will overflow silently and the wrong feeder will be removed when you try to remove asset.

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

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

Recommended Mitigation Steps

Check and limit the length of feeder array or use uint256 for FeederRegistrar.index

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/marketplaces/LooksRareAdapter.sol#L73-L94 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/marketplaces/SeaportAdapter.sol#L93-L107 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/marketplaces/X2Y2Adapter.sol#L88-L102

Vulnerability details

Impact

matchAskWithTakerBid is payable, it call functionCallWithValue with the value parameter. There are no check to make sure msg.value == value`, if excess value is sent user will not receive a refund.

Proof of Concept

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/marketplaces/LooksRareAdapter.sol#L73-L94

    function matchAskWithTakerBid(
        address marketplace,
        bytes calldata params,
        uint256 value
    ) external payable override returns (bytes memory) {
        bytes4 selector;
        if (value == 0) {
            selector = ILooksRareExchange.matchAskWithTakerBid.selector;
        } else {
            selector = ILooksRareExchange
                .matchAskWithTakerBidUsingETHAndWETH
                .selector;
        }
        bytes memory data = abi.encodePacked(selector, params);
        return
            Address.functionCallWithValue(
                marketplace,
                data,
                value,
                Errors.CALL_MARKETPLACE_FAILED
            );
    }

Same code exists in LooksRareAdapter, SeaportAdapter and X2Y2Adapter

Recommended Mitigation Steps

Check msg.value == value if the function is only supposed to be delegate called into, consider add a check to prevent direct call.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/ParaSpaceOracle.sol#L128-L129

Vulnerability details

Impact

Chainlink latestAnswer() is deprecated. It might return stale data or incomplete round answer.

Proof of Concept

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

 * - Use of Chainlink Aggregators as first source of price

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

            price = uint256(source.latestAnswer());

Recommended Mitigation Steps

Check for stale price and round completeness using latestRoundData()

Low

MAX_VALID_LTV is set above 100%

It does not make sense to allow LTV above 100%, although it is currently not possible as a consequence of other invariant, it is recommended to set MAX_VALID_LTV to PERCENTAGE_FACTOR.

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/configuration/ReserveConfiguration.sol#L48

    uint256 internal constant MAX_VALID_LTV = 65535;

Currently, it is not possible to set ltv > PERCENTAGE_FACTOR because the following code make sure ltv <= liquidationThreshold <= PERCENTAGE_FACTOR

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/pool/PoolConfigurator.sol#L141-L160

        require(ltv <= liquidationThreshold, Errors.INVALID_RESERVE_PARAMS);

        DataTypes.ReserveConfigurationMap memory currentConfig = _pool
            .getConfiguration(asset);

        if (liquidationThreshold != 0) {
            //liquidation bonus must be bigger than 100.00%, otherwise the liquidator would receive less
            //collateral than needed to cover the debt
            require(
                liquidationBonus >= PercentageMath.PERCENTAGE_FACTOR,
                Errors.INVALID_RESERVE_PARAMS
            );

            //if threshold * bonus is less than PERCENTAGE_FACTOR, it's guaranteed that at the moment
            //a loan is taken there is enough collateral available to cover the liquidation bonus
            require(
                liquidationThreshold.percentMul(liquidationBonus) <=
                    PercentageMath.PERCENTAGE_FACTOR,
                Errors.INVALID_RESERVE_PARAMS
            );

Price can be equal to 0 while being valid

ParaSpaceOracle revert if both the primary and fallback oracle return price == 0. This might have undesired consequence, for instance, if an asset price literally drop to 0, the protocol will not be able to liquidate the said asset.

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

        require(price != 0, Errors.ORACLE_PRICE_NOT_READY);

Dangerous delegate call pattern

The pool delegate call into the adapter logics to execute certain action, project admin / governance need to be very careful when approval new marketplace / adapter since it might brick the protocol if it modify storage slot / call selfdestruct.

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/MarketplaceLogic.sol#L132-L133

        Address.functionDelegateCall(
            params.marketplace.adapter,

Non-Critical

Confusing variable naming

For example, liquidationProtocolFee and liquidationProtocolFeePercentage are used interchangeably where it is unclear it represent the amount or %.

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

        vars.liquidationProtocolFeePercentage = collateralReserve
            .configuration
            .getLiquidationProtocolFee();

Unused error code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/helpers/Errors.sol#L15

    string public constant CALLER_NOT_BRIDGE = "6"; // 'The caller of the function is not a bridge'

Dangerous ReserveConfiguration design

ReserveConfiguration used a bit masking appoach to pack storage. While no issue is found at the moment it is a risky design. Consider using a packed struct instead.

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/configuration/ReserveConfiguration.sol#L12-L13

Unresolved TODOs

contracts/mocks/tokens/Moonbirds.sol:483:OpenSea's example code. TODO: it's likely that the above mapping can be changed
contracts/misc/UniswapV3OracleWrapper.sol:238:        // TODO using bit shifting for the 2^96
contracts/misc/marketplaces/LooksRareAdapter.sol:59:            makerAsk.price, // TODO: take minPercentageToAsk into account
contracts/ui/UiIncentiveDataProvider.sol:68:            // TODO: check that this is deployed correctly on contract and remove casting
contracts/protocol/libraries/logic/MarketplaceLogic.sol:442:        // TODO: support PToken
contracts/interfaces/INToken.sol:97:    // TODO are we using the Treasury at all? Can we remove?