#7 status() function can return stale data or the L2 sequencer could be down, allowing minted > maxMintable() in reality or an incorrect distributeAssets()

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L94

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L75

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L67

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L99

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L156

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L83

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L127

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L206

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L207-L218

Summary

distributeAssets() fetches latestRoundData from chainlink which is never checked for staleness or an incomplete round or if the L2 sequencer is down and as such can result in incorrect rewards/assets being distributed, since the protocol could be relying on outdated values
Similarly, the following functions all make use of the variable calculator = IPriceCalculator(_priceCalculator) present on L40 to fetch latestRoundData from chainlink. These can result in the user minting more tokens than the allowed maxMintable, since the protocol could be relying on outdated values:
Additionally, there's the issue of "Unhandled Chainlink Revert - Denial Of Service" as outlined in Cyfrin's article as no try/catch blocks have been used in functions like distributeAssets() while calling latestRoundData().

Vulnerability Details

Unchecked chainlink data being used inside distributeAssets():

    function distributeAssets(ILiquidationPoolManager.Asset[] memory _assets, uint256 _collateralRate, uint256 _hundredPC) external payable {
        consolidatePendingStakes();
@--->   (,int256 priceEurUsd,,,) = Chainlink.AggregatorV3Interface(eurUsd).latestRoundData();
        uint256 stakeTotal = getStakeTotal();
        uint256 burnEuros;
        uint256 nativePurchased;
        for (uint256 j = 0; j < holders.length; j++) {
            Position memory _position = positions[holders[j]];
            uint256 _positionStake = stake(_position);
            if (_positionStake > 0) {
                for (uint256 i = 0; i < _assets.length; i++) {
                    ILiquidationPoolManager.Asset memory asset = _assets[i];
                    if (asset.amount > 0) {
@--------->             (,int256 assetPriceUsd,,,) = Chainlink.AggregatorV3Interface(asset.token.clAddr).latestRoundData();
                        uint256 _portion = asset.amount * _positionStake / stakeTotal;
                        ....
                        ....

calculator variable is used to call tokenToEurAvg(), tokenToEur() and eurToToken() which internally call latestRoundData() on L47, L56 and L63 but never check if the data is stale.
Hence, maxMintable() can be an outdated value resulting in the user able to remove more collateral than they should be able to or even minting more than they should be able to.
Additionally, calls to Chainlink could revert, which may result in a complete Denial-of-Service as outlined in OZ blog. Chainlink multisigs can immediately block access to price feeds at will, so just because a price feed is working today does not mean it will continue to do so indefinitely. Smart contracts should handle this by wrapping calls to Oracles in try/catch blocks and dealing appropriately with any errors.

    function euroCollateral() private view returns (uint256 euros) {
        ITokenManager.Token[] memory acceptedTokens = getTokenManager().getAcceptedTokens();
        for (uint256 i = 0; i < acceptedTokens.length; i++) {
            ITokenManager.Token memory token = acceptedTokens[i];
@------>    euros += calculator.tokenToEurAvg(token, getAssetBalance(token.symbol, token.addr));
        }
    }

    function maxMintable() private view returns (uint256) {
@--->   return euroCollateral() * ISmartVaultManagerV3(manager).HUNDRED_PC() / ISmartVaultManagerV3(manager).collateralRate();
    }

    function canRemoveCollateral(ITokenManager.Token memory _token, uint256 _amount) private view returns (bool) {
        if (minted == 0) return true;
@--->   uint256 currentMintable = maxMintable();
        uint256 eurValueToRemove = calculator.tokenToEurAvg(_token, _amount);
        return currentMintable >= eurValueToRemove &&
            minted <= currentMintable - eurValueToRemove;
    }

    function mint(address _to, uint256 _amount) external onlyOwner ifNotLiquidated {
        uint256 fee = _amount * ISmartVaultManagerV3(manager).mintFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
@--->   require(fullyCollateralised(_amount + fee), UNDER_COLL);
        minted = minted + _amount + fee;
        EUROs.mint(_to, _amount);
        EUROs.mint(ISmartVaultManagerV3(manager).protocol(), fee);
        emit EUROsMinted(_to, _amount, fee);
    }

Impact

Use of stale values (or a down L2 sequencer, or a Chainlink revert) can

result in breaking the invariant that the user should not be able to mint more than maxMintable.
result in user being able to remove more than allowed collateral.
result in either more or less than expected rewards/assets transferred via distributeAssets().
result in a complete Denial-of-Service to those calling distributeAssets().

Tools Used

Manual inspection

Recommendations

While fetching the latestRoundData in PriceCalculator.sol L47, L56 and L63, add the following checks each time. Similar check needs to be added inside distributeAssets():

-       (, int256 eurUsdPrice,,,) = clEurUsd.latestRoundData();
+       (uint80 roundId, int256 eurUsdPrice,, uint256 updatedAt, uint80 answeredInRound) = clEurUsd.latestRoundData();
+       require(eurUsdPrice > minAnswer && eurUsdPrice < maxAnswer, "Chainlink price outside min & max bounds"); // @audit : protocol should set reasonable limits for `minAnswer` & `maxAnswer` beforehand
+       require(updatedAt > block.timestamp - staleTimePeriod[tokenPair][network], "Stale price"); // @audit : set `staleTimePeriod` to some value based on the network & `heartbeat` of that token-pair. See here: https://docs.chain.link/data-feeds/price-feeds/addresses/?network=arbitrum&page=1
+       require(answeredInRound >= roundId, "Stale price");

Also, we need to check if the L2 sequencer is up before fetching the above price feed. Code needs to be added as per: https://docs.chain.link/data-feeds/l2-sequencer-feeds

        (
            /*uint80 roundID*/,
            int256 answer,
            uint256 startedAt,
            /*uint256 updatedAt*/,
            /*uint80 answeredInRound*/
        ) = sequencerUptimeFeed.latestRoundData();

        // Answer == 0: Sequencer is up
        // Answer == 1: Sequencer is down
        bool isSequencerUp = answer == 0;
        if (!isSequencerUp) {
            revert SequencerDown();
        }

        // Make sure the grace period has passed after the
        // sequencer is back up.
        uint256 timeSinceUp = block.timestamp - startedAt;
        if (timeSinceUp <= GRACE_PERIOD_TIME) {
            revert GracePeriodNotOver();
        }

In addition to the above, surround the chainlink calls with try/catch as explained here.