Stale or incorrect results from data feeds can affect assets and shares calculation on deposits and withdrawals

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L646-L649
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L164-L165
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L676
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L679-L686
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L616-L618
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderStakePoolsManager.sol#L271-L277
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderStakePoolsManager.sol#L294-L298
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderStakePoolsManager.sol#L169-L177
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/UserWithdrawalManager.sol#L95-L111

Vulnerability details

StaderOracle uses data feeds for checking ETH balance and ETHX supply values, but it doesn't perform a validation to check their correctness, and can result in incorrect results or stale data.

Those values are used to update the exchangeRate storage value from the StaderOracle.

And ultimately used by the StaderStakePoolsManager to calculate assets and shares conversions for deposits and withdrawals. Thus the importance of accurate values.

Impact

Incorrect calculation of amount of shares and assets on deposits and withdrawals from StaderStakePoolsManager, and UserWithdrawalManager at the expense of either the user or the protocol.

Proof of Concept

StaderOracle does not check for stale results or their correctness on the latestRoundData(). These values come from Data Feeds.

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

Link to code

These values are retrieved and updated on updateERFromPORFeed():

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

Link to code

Which calls _updateExchangeRate:

    _updateExchangeRate(_newTotalETHBalance, _newTotalETHXSupply, _reportingBlockNumber);

Link to code

And updates the exchangeRate storage variable:

    function _updateExchangeRate(
        uint256 _totalETHBalance,
        uint256 _totalETHXSupply,
        uint256 _reportingBlockNumber
    ) internal {
        exchangeRate.totalETHBalance = _totalETHBalance;
        exchangeRate.totalETHXSupply = _totalETHXSupply;
        exchangeRate.reportingBlockNumber = _reportingBlockNumber;

Link to code

This value will be retrieved via getExchangeRate():

    function getExchangeRate() external view override returns (ExchangeRate memory) {
        return (exchangeRate);
    }

Link to code

The contract that retrieves that value is the StaderStakePoolsManager.

It is ultimately used to calculate the previewDeposit() and the previewWithdraw():

// File: StaderStakePoolsManager.sol

    function _convertToShares(uint256 _assets, Math.Rounding rounding) internal view returns (uint256) {
        uint256 supply = IStaderOracle(staderConfig.getStaderOracle()).getExchangeRate().totalETHXSupply;
        return
            (_assets == 0 || supply == 0)
                ? initialConvertToShares(_assets, rounding)
                : _assets.mulDiv(supply, totalAssets(), rounding);
    }

    function previewDeposit(uint256 _assets) public view override returns (uint256) {
        return _convertToShares(_assets, Math.Rounding.Down);
    }

Link to code

    function _convertToAssets(uint256 _shares, Math.Rounding rounding) internal view returns (uint256) {
        uint256 supply = IStaderOracle(staderConfig.getStaderOracle()).getExchangeRate().totalETHXSupply;
        return
            (supply == 0) ? initialConvertToAssets(_shares, rounding) : _shares.mulDiv(totalAssets(), supply, rounding);
    }

    function previewWithdraw(uint256 _shares) public view override returns (uint256) {
        return _convertToAssets(_shares, Math.Rounding.Down);
    }

Link to code

previewDeposit() is used on the deposit() function to calculate the shares that will be minted to the depositor:

    function deposit(address _receiver) public payable override whenNotPaused returns (uint256) {
        uint256 assets = msg.value;
        if (assets > maxDeposit() || assets < minDeposit()) {
            revert InvalidDepositAmount();
        }
        uint256 shares = previewDeposit(assets);
        _deposit(msg.sender, _receiver, assets, shares);
        return shares;
    }

    function _deposit(
        address _caller,
        address _receiver,
        uint256 _assets,
        uint256 _shares
    ) internal {
        ETHx(staderConfig.getETHxToken()).mint(_receiver, _shares);
        emit Deposited(_caller, _receiver, _assets, _shares);
    }

Link to code

previewWithdraw() is used by UserWithdrawalManager to calculate the assets, and ultimately the ethExpected to be received.

    function requestWithdraw(uint256 _ethXAmount, address _owner) external override whenNotPaused returns (uint256) {
        if (_owner == address(0)) revert ZeroAddressReceived();
        uint256 assets = IStaderStakePoolManager(staderConfig.getStakePoolManager()).previewWithdraw(_ethXAmount); // @audit
        if (assets < staderConfig.getMinWithdrawAmount() || assets > staderConfig.getMaxWithdrawAmount()) {
            revert InvalidWithdrawAmount();
        }
        if (requestIdsByUserAddress[_owner].length + 1 > maxNonRedeemedUserRequestCount) {
            revert MaxLimitOnWithdrawRequestCountReached();
        }
        IERC20Upgradeable(staderConfig.getETHxToken()).safeTransferFrom(msg.sender, (address(this)), _ethXAmount);
        ethRequestedForWithdraw += assets;
        userWithdrawRequests[nextRequestId] = UserWithdrawInfo(payable(_owner), _ethXAmount, assets, 0, block.number);
        requestIdsByUserAddress[_owner].push(nextRequestId);
        emit WithdrawRequestReceived(msg.sender, _owner, nextRequestId, _ethXAmount, assets);
        nextRequestId++;
        return nextRequestId - 1;
    }

Link to code

Assets and shares calculation on deposits and withdrawals are at the expense of the user or the protocol, and thus the importance of their accuracy.

Tools Used

Manual Review

Recommended Mitigation Steps

Verify the results returned on the latestRoundData, checking roundId, updatedAt for stale results, and answer for incorrect results as 0.

Assessed type

Oracle

[c4-judge]

Picodes marked the issue as duplicate of #15

[c4-judge]

Picodes marked the issue as satisfactory