Stader Labs - Bauchibred's results

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderOracle.sol#L637-L677 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderOracle.sol#L159-L166

Vulnerability details

Impact

The updateERFromPORFeed() and getPORFeedData() functions in the StaderOracle.sol contract are vulnerable to a Denial of Service (DoS) attack. If a call to latestRoundData() reverts for any reason - for instance, if the oracle goes down or the multisigs decide to stop providing data via this oracle - it could halt the execution of these functions. This would effectively freeze any operations relying on these functions, potentially rendering parts of the protocol dysfunctional.

Proof of Concept

The updateERFromPORFeed() function calls the getPORFeedData() function, which retrieves data from a Chainlink oracle using latestRoundData(). If for any reason, the oracle ceases to provide data or experiences an outage, the call to latestRoundData() would revert, causing a halt in the execution of getPORFeedData() and, by extension, updateERFromPORFeed().

Take a look at both functions from StaderOracle.sol

    function updateERFromPORFeed() external override checkERInspectionMode whenNotPaused {
        if (!isPORFeedBasedERData) {
            revert InvalidERDataSource();
        }

        (uint256 newTotalETHBalance, uint256 newTotalETHXSupply, uint256 reportingBlockNumber) = getPORFeedData();
        updateWithInLimitER(newTotalETHBalance, newTotalETHXSupply, reportingBlockNumber);
    }

    function getPORFeedData()
        internal
        view
        returns (
            uint256,
            uint256,
            uint256
        )
    {

        (, int256 totalETHBalanceInInt, , , ) = AggregatorV3Interface(staderConfig.getETHBalancePORFeedProxy())
            .latestRoundData();

        (, int256 totalETHXSupplyInInt, , , ) = AggregatorV3Interface(staderConfig.getETHXSupplyPORFeedProxy())
            .latestRoundData();
        return (uint256(totalETHBalanceInInt), uint256(totalETHXSupplyInInt), block.number);
    }

Tool used

Manual Review

Recommendation

Consider introducing a try/catch block around the latestRoundData() calls. If these calls revert, the catch block should contain logic to handle the failure. This could be a fallback mechanism, an alternative oracle call, or a contingency procedure to pause operations and alert protocol administrators.

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderOracle.sol#L637-L677 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderOracle.sol#L159-L166

Vulnerability details

Impact

The potential misuse arises when the actual price of an asset drastically changes, but the protocol continues operating with the price that could be at the minAnswer or maxAnswer. This could enable manipulative behaviors, posing a risk to the protocol's financial stability.

Proof of Concept

Chainlink aggregators have a built in circuit breaker if the price of an asset goes outside of a predetermined price band. The result is that if an asset experiences a huge drop in value (i.e. LUNA crash) the price of the oracle will continue to return the minPrice instead of the actual price of the asset. This would allow user to continue borrowing with the asset but at the wrong price. This is exactly what happened to Venus on BSC when LUNA drastically imploded. Note that the function updateERFromPORFeed() and getPORFeedData() in the StaderOracle.sol contract, do not implement any mechanism to check if the price returned is equal to minAnswer or maxAnswer.

Take a look at both functions from StaderOracle.sol

    function updateERFromPORFeed() external override checkERInspectionMode whenNotPaused {
        if (!isPORFeedBasedERData) {
            revert InvalidERDataSource();
        }
        (uint256 newTotalETHBalance, uint256 newTotalETHXSupply, uint256 reportingBlockNumber) = getPORFeedData();
        updateWithInLimitER(newTotalETHBalance, newTotalETHXSupply, reportingBlockNumber);
    }

    function getPORFeedData()
        internal
        view
        returns (
            uint256,
            uint256,
            uint256
        )
    {

        (, int256 totalETHBalanceInInt, , , ) = AggregatorV3Interface(staderConfig.getETHBalancePORFeedProxy())
            .latestRoundData();

        (, int256 totalETHXSupplyInInt, , , ) = AggregatorV3Interface(staderConfig.getETHXSupplyPORFeedProxy())
            .latestRoundData();
        return (uint256(totalETHBalanceInInt), uint256(totalETHXSupplyInInt), block.number);
    }

Here is an Example explaining the issue:

TokenA has a minPrice of $1. The price of TokenA drops to $0.10. The aggregator still returns $1 allowing the user to interact with TokenA as if it is $1 which is 10x it's actual value.

Tool used

Manual Review

Recommendation

The updateERFromPORFeed() function should be modified to include a validation check. If the returned price is equal to minAnswer or maxAnswer, the function should revert to avoid operating on incorrect prices.

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderOracle.sol#L637-L677 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderOracle.sol#L159-L166

Vulnerability details

Impact

The absence of a freshness check in the getPORFeedData() function might lead to stale data being used, potentially causing inaccurate calculations and impacting the functioning of the contract. Stale data could be a result of delayed updates or missed updates due to network congestion or similar issues. This could lead to undesired effects such as incorrect token valuations or transaction handling, potentially causing financial losses for users or compromising the integrity of the contract.

Proof of Concept

In the getPORFeedData() function in the StaderOracle.sol contract, there is no freshness check on the data received from the latestRoundData() function calls. The code only fetches the data without validating its recency:

function getPORFeedData()
    internal
    view
    returns (
        uint256,
        uint256,
        uint256
    )
{
//@audit ... code blocks ommited for brevity
    (, int256 totalETHBalanceInInt, , , ) = AggregatorV3Interface(staderConfig.getETHBalancePORFeedProxy())
        .latestRoundData();
    (, int256 totalETHXSupplyInInt, , , ) = AggregatorV3Interface(staderConfig.getETHXSupplyPORFeedProxy())
        .latestRoundData();
    return (uint256(totalETHBalanceInInt), uint256(totalETHXSupplyInInt), block.number);
}

This data is then used in updateERFromPORFeed() function, potentially propagating the use of stale data:

function updateERFromPORFeed() external override checkERInspectionMode whenNotPaused {
   //@audit ... code blocks ommited for brevity
    if (!isPORFeedBasedERData) {
        revert InvalidERDataSource();
    }
    (uint256 newTotalETHBalance, uint256 newTotalETHXSupply, uint256 reportingBlockNumber) = getPORFeedData();
    updateWithInLimitER(newTotalETHBalance, newTotalETHXSupply, reportingBlockNumber);
}

Tool used

Manual Review

Recommendation

To address this vulnerability, it is recommended to implement a freshness check for the data obtained from the latestRoundData() function. This could be accomplished by comparing the timestamp of the latest data to the current block timestamp. If the difference exceeds a certain threshold, the data should be considered stale and the operation should be aborted or an alternative course of action should be taken. This freshness check should be conducted in the getPORFeedData() function to ensure that only fresh data is used for subsequent operations.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderOracle.sol#L637-L677 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderOracle.sol#L159-L166

Vulnerability details

Impact

The use of negative prices can cause serious issues in the calculation and representation of asset values within the protocol. This can lead to a multitude of problems, such as distorted collateral valuation, wrong rate minting, and potential exploitative behaviors, posing a risk to the protocol's financial stability.

TLDR: Invalid price is used, espeically when price goes to negative and the sliently casting to uint256, the collateral valuation will be wrong.

NB: Asset prices can definitely go negative due to carying costs.

Proof of Concept

Take a look at both functions from StaderOracle.sol

    function updateERFromPORFeed() external override checkERInspectionMode whenNotPaused {
        if (!isPORFeedBasedERData) {
            revert InvalidERDataSource();
        }
        //@audit -ve prices are casted into uint256
        (uint256 newTotalETHBalance, uint256 newTotalETHXSupply, uint256 reportingBlockNumber) = getPORFeedData();
        updateWithInLimitER(newTotalETHBalance, newTotalETHXSupply, reportingBlockNumber);
    }

    function getPORFeedData()
        internal
        view
        returns (
            uint256,
            uint256,
            uint256
        )
    {
        //@audit -ve prices could be returned
        (, int256 totalETHBalanceInInt, , , ) = AggregatorV3Interface(staderConfig.getETHBalancePORFeedProxy())
            .latestRoundData();
        //@audit -ve prices could be returned
        (, int256 totalETHXSupplyInInt, , , ) = AggregatorV3Interface(staderConfig.getETHXSupplyPORFeedProxy())
            .latestRoundData();
        return (uint256(totalETHBalanceInInt), uint256(totalETHXSupplyInInt), block.number);
    }

As seen a call to the updateERFromPORFeed() function queries the getPORFeedData() function which in turn retrieves the latest round data from the Chainlink Feed Registry, but doesn't seem to have mechanisms to handle negative prices. If a negative price is returned, it is silently cast to uint256 and used in the updateERFromPORFeed() function, which could lead to an exploitation scenario where the protocol interacts with the asset using incorrect price information.

Tool used

Manual Review

Recommendation

The protocol should include a validation check in updateERFromPORFeed() to ensure that the price returned from getPORFeedData() is not negative. If a negative price is encountered, the function should revert to avoid further operation with potentially incorrect price data, or the check could be implemented in the getPORFeedData() function itself

Assessed type

Invalid Validation