sherlock-audit / 2023-12-arcadia-judging

18 stars 15 forks source link

ast3ros - Missing sequencerNotDown modifier in getValuesInUsdRecursive #191

Closed sherlock-admin2 closed 7 months ago

sherlock-admin2 commented 7 months ago

ast3ros

medium

Missing sequencerNotDown modifier in getValuesInUsdRecursive

Summary

The absence of the sequencerNotDown modifier in the getValuesInUsdRecursive function can result in the incorrect calculation of underlying amounts for UniswapV3 positions.

Vulnerability Detail

When UniswapV3 asset module calculates the underlying assets' amounts of a position, it calls _getRateUnderlyingAssetsToUsd.

    function _getUnderlyingAssetsAmounts(address creditor, bytes32 assetKey, uint256 amount, bytes32[] memory) 
        internal
        view
        override
        returns (uint256[] memory underlyingAssetsAmounts, AssetValueAndRiskFactors[] memory rateUnderlyingAssetsToUsd)
    {
        ...

        // Get the trusted rates to USD of the Underlying Assets.
        bytes32[] memory underlyingAssetKeys = new bytes32[](2);
        underlyingAssetKeys[0] = _getKeyFromAsset(token0, 0);
        underlyingAssetKeys[1] = _getKeyFromAsset(token1, 0);
        rateUnderlyingAssetsToUsd = _getRateUnderlyingAssetsToUsd(creditor, underlyingAssetKeys);
        ...
    }

https://github.com/arcadia-finance/accounts-v2/blob/83eef2ef44a46a19e46b3d007929b5ea64db4789/src/asset-modules/UniswapV3/UniswapV3AM.sol#L185-L230

The _getRateUnderlyingAssetsToUsd in turn calls Registry.getValuesInUsdRecursive.

    function _getRateUnderlyingAssetsToUsd(address creditor, bytes32[] memory underlyingAssetKeys)
        internal
        view
        virtual
        returns (AssetValueAndRiskFactors[] memory rateUnderlyingAssetsToUsd)
    {
        uint256 length = underlyingAssetKeys.length;

        address[] memory underlyingAssets = new address[](length);
        uint256[] memory underlyingAssetIds = new uint256[](length);
        uint256[] memory amounts = new uint256[](length);
        for (uint256 i; i < length; ++i) {
            (underlyingAssets[i], underlyingAssetIds[i]) = _getAssetFromKey(underlyingAssetKeys[i]);
            // We use the USD price per 10**18 tokens instead of the USD price per token to guarantee
            // sufficient precision.
            amounts[i] = 1e18;
        }

        rateUnderlyingAssetsToUsd =
            IRegistry(REGISTRY).getValuesInUsdRecursive(creditor, underlyingAssets, underlyingAssetIds, amounts);
    }

https://github.com/arcadia-finance/accounts-v2/blob/83eef2ef44a46a19e46b3d007929b5ea64db4789/src/asset-modules/abstracts/AbstractDerivedAM.sol#L96-L116

The getValuesInUsdRecursive calls the underlying asset modules to get the asset prices.

    function getValuesInUsdRecursive(
        address creditor,
        address[] calldata assets,
        uint256[] calldata assetIds,
        uint256[] calldata assetAmounts
    ) external view returns (AssetValueAndRiskFactors[] memory valuesAndRiskFactors) {
        uint256 length = assets.length;
        valuesAndRiskFactors = new AssetValueAndRiskFactors[](length);
        for (uint256 i; i < length; ++i) {
            (
                valuesAndRiskFactors[i].assetValue,
                valuesAndRiskFactors[i].collateralFactor,
                valuesAndRiskFactors[i].liquidationFactor
            ) = IAssetModule(assetToAssetModule[assets[i]]).getValue(creditor, assets[i], assetIds[i], assetAmounts[i]);
        }
    }

https://github.com/arcadia-finance/accounts-v2/blob/05ed2ab39c97f86626dda0062356161bf30c82d7/src/Registry.sol#L616-L631

However in the getValuesInUsdRecursive, it lacks the sequencerNotDown modifier. Therefore, a stale orcale is allowed to consumed and leads to the wrong calculation of UniswapV3 position underlying amount.

Impact

UniswapV3 position underlying amounts are inaccurate when the Sequencer is down. It leads to wrong valuation of the Uniswap V3 Liquidity Positions

Code Snippet

https://github.com/arcadia-finance/accounts-v2/blob/05ed2ab39c97f86626dda0062356161bf30c82d7/src/Registry.sol#L616-L631

Tool used

Manual Review

Recommendation

Add sequencerNotDown(creditor) to the getValuesInUsdRecursive function.

Duplicate of #105

sherlock-admin2 commented 7 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid