Stader Labs - dwward3n's results

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/VaultProxy.sol#L17

Vulnerability details

Impact

Attacker can selfdestruct VaultProxy deployment.

    constructor() {}

    //initialise the vault proxy with data
    function initialise(
        bool _isValidatorWithdrawalVault,
        uint8 _poolId,
        uint256 _id,
        address _staderConfig
    ) external {
        if (isInitialized) {
            revert AlreadyInitialized();
        }
        UtilLib.checkNonZeroAddress(_staderConfig);
        isValidatorWithdrawalVault = _isValidatorWithdrawalVault;
        isInitialized = true;
        poolId = _poolId;
        id = _id;
        staderConfig = IStaderConfig(_staderConfig);
        owner = staderConfig.getAdmin();
    }

    /**route all call to this proxy contract to the respective latest vault contract
     * fetched from staderConfig. This approch will help in changing the implementation
     * of validatorWithdrawalVault/nodeELRewardVault for already deployed vaults*/
    fallback(bytes calldata _input) external payable returns (bytes memory) {
        address vaultImplementation = isValidatorWithdrawalVault
            ? staderConfig.getValidatorWithdrawalVaultImplementation()
            : staderConfig.getNodeELRewardVaultImplementation();
        (bool success, bytes memory data) = vaultImplementation.delegatecall(_input);
        if (!success) {
            revert(string(data));
        }
        return data;
    }

VaultProxy is first deployed using deploy script (deployContracts.ts). Since anyone can call initialise, attacker can craft malicious parameters for later fallback call. In the fallback, implementation address is get from staderConfig which is initialized in initialise() thus attacker have full control over it. Attacker can deploy a contract containing selfdestruct and call it via fallback.

Since VaultProxy is used in VaultFactory for deploying vaults, this will cause service disrupture. Admin can try to solve this by VaultFactory::updateVaultProxyAddress with another VaultProxy, but because VaultProxy::initialise can be frontrun, determined attacker can destroy that VaultProxy too.

Proof of Concept

contract Destructible {
    function exit() external {
        selfdestruct(payable(msg.sender));
    }
}

contract FakeStaderConfig {
    ...
    function getValidatorWithdrawalVaultImplementation() external returns (address) {
        // return address of deployed Destructible
        ...
    }
}

interface IVaultProxy {
    function initialise(bool, uint, uint, address) external;
}

contract Attack {
    ...
    function attack() external {
        IVaultProxy(vaultProxy).initialise(
            true,   // _isValidatorWithdrawalVault
            0,      // any
            0,      // any
            ...     // address of deployed FakeStaderConfig
        );

        vaultProxy.call(abi.encodeWithSelector(Destructible.exit.selector));
    }
}

Tools Used

Manual

Recommended Mitigation Steps

Add guard for VaultProxy::initialise.

>>  address immutable factory;

>>  constructor() { factory = msg.sender; }

    //initialise the vault proxy with data
    function initialise(
        bool _isValidatorWithdrawalVault,
        uint8 _poolId,
        uint256 _id,
        address _staderConfig
    ) external {
>>      require(msg.sender == factory);
        if (isInitialized) {
            revert AlreadyInitialized();
        }

Assessed type

call/delegatecall

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L148 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L290

Vulnerability details

Impact

        if (
            submissionCount == trustedNodesCount / 2 + 1 &&
            _exchangeRate.reportingBlockNumber > exchangeRate.reportingBlockNumber
        ) {
            updateWithInLimitER(
                _exchangeRate.totalETHBalance,
                _exchangeRate.totalETHXSupply,
                _exchangeRate.reportingBlockNumber
            );
        }

In submitExchangeRateData, consensus is reached if submissionCount is strictly equal to desired number. However, trustNodesCount can be decreased and this condition can be never met.

        if ((submissionCount == (2 * trustedNodesCount) / 3 + 1)) {
            lastReportedSDPriceData = _sdPriceData;
            lastReportedSDPriceData.sdPriceInETH = getMedianValue(sdPrices);
            delete sdPrices;

In submitSDPrice, if this case happens, sdPrices doesn't get deleted and it will affect the next submission batch's price.

Proof of Concept

In the above snippet, let's assume trustedNodesCount = 10, submissionCount = 5. The condition doesn't meet for now (5 != 10/2+1). Then trustedNodesCount decreases to 9. Next time when a node submits, trustedNodesCount = 9, submissionCount = 6. Then the condition cannot be met since 6 != 9/2+1.

Tools Used

Manual

Recommended Mitigation Steps

Replace strict equal with equal or greater than. Or replace it with greater than and decrease the right side.

Not sure about adding cooldown for add/remove trusted nodes.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L646 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L648

Vulnerability details

Impact

In StaderOracle::getPORFeedData, latestRoundData is used, but there is no check if the return value indicates stale data.

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

This could lead to stale prices according to the Chainlink documentation: https://docs.chain.link/data-feeds/historical-data

Proof of Concept

Tools Used

Manual

Recommended Mitigation Steps

(uint80 roundID, int256 answer, , uint256 updatedAt, uint80 answeredInRound) These are return parameters of latestRoundData, so can add checks like below.

        ...
        require(answeredInRound >= roundID, "Stale price");

Assessed type

Oracle