Stader Labs - ChrisTina's results

Lines of code

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

Vulnerability details

Impact

in the updateAdmin function, the DEFAULT_ADMIN_ROLE is first granted to the new admin and then revoked from the old admin. If both the new admin and the old admin have the same value (admin updates admin to self for some reason), the admin role will be revoked, leaving the contract without a default admin.

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

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

While unlikely, the situation where the admin updates the admin to himself is often present in the test cases. This didn't really present a problem because the "original" admin is not added to the accountsMap during initialization and therefore his role is not being revoked, as oldAdmin is the zero address (see my previous submission). However, if the previous admin exists in the accounts map (=> the updateAdmin function has been called before) and then the admin updates admin to himself, he will revoke his own admin privileges.

Proof of Concept

pragma solidity 0.8.16;
import 'forge-std/Test.sol';
import '../../contracts/StaderConfig.sol';
import '@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol';

contract AdminTest is Test {
  address staderAdmin = vm.addr(100);
  address staderAdmin2 = vm.addr(101);
  address proxyAdmin = vm.addr(102);
  address ethDepositAddr = vm.addr(103);
  StaderConfig staderConfig;

  function setUp() public {
    StaderConfig configImpl = new StaderConfig();
    TransparentUpgradeableProxy configProxy = new TransparentUpgradeableProxy(
      address(configImpl),
      address(proxyAdmin),
      ''
    );
    staderConfig = StaderConfig(address(configProxy));
    staderConfig.initialize(staderAdmin, ethDepositAddr);
    // staderAdmin hands over to staderAdmin2 as default admin
    vm.startPrank(staderAdmin);
    staderConfig.updateAdmin(staderAdmin2);
  }

  /**
   * staderAdmin2 revokes own DEFAULT_ADMIN_ROLE by updating admin to self
   */
  function test_AdminRevokesOwnRole() public {
    assertEq(staderConfig.getAdmin(), staderAdmin2);
    vm.startPrank(staderAdmin2);
    // does admin stuff
    staderConfig.updateMaxWithdrawAmount(10001 ether);
    // updates admin to self
    staderConfig.updateAdmin(staderAdmin2);
    assertEq(staderConfig.getAdmin(), staderAdmin2);
    // tries to do more admin stuff but can't --> test fails
    staderConfig.updateMaxWithdrawAmount(10002 ether);
  }
}

Tools Used

Foundry

Recommended Mitigation Steps

To ordering should be changed to first revoking and then granting the role.

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

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/Auction.sol#L1 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/OperatorRewardsCollector.sol#L15 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/SocializingPool.sol#L21

Vulnerability details

Impact

The contract doesn't have any public facing pause / unpause functions and therefore can not be paused, e.g. for an upgrade. Also applies to SocializingPool.sol and OperatorRewardsCollector.sol

Proof of Concept

Auction.sol extends PausableUpgradeable

contract Auction is IAuction, Initializable, AccessControlUpgradeable, PausableUpgradeable, ReentrancyGuardUpgradeable

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

and uses the whenNotPaused modifier on several functions

 function createLot(uint256 _sdAmount) external override whenNotPaused

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

function addBid(uint256 lotId) external payable override whenNotPaused

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

but doesn't have any function, e.g. an external pause function, that triggers the internal _pause function.

The contract is initialized to _paused = false; and can never change to a paused state.

Tools Used

/

Recommended Mitigation Steps

Add an external pause and unpause function with the proper access restrictions, similar to https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#L188-L203

/**
 * @dev Triggers stopped state.
 * Contract must not be paused
 */
function pause() external {
  UtilLib.onlyManagerRole(msg.sender, staderConfig);
  _pause();
}

/**
 * @dev Returns to normal state.
 * Contract must be paused
 */
function unpause() external onlyRole(DEFAULT_ADMIN_ROLE) {
  _unpause();
}

Assessed type

Upgradable

Lines of code

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

Vulnerability details

Impact

During initialization of the StaderConfig contract, the _admin is granted the _DEFAULT_ADMIN_ROLE, but not added to the accountsMap. https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderConfig.sol#L102

As a result, the getAdmin function doesn't return the admin address, but the 0 address.

function getAdmin() external view returns (address) {
  return accountsMap[ADMIN];
}

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

This value is used in VaultProxy.sol to determine the owner of the VaultProxy => the owner will be set to the 0 address.

owner = staderConfig.getAdmin();

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

The only functions restricted to the owner are setting a new owner and updating the stader config. https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/VaultProxy.sol#L57 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/VaultProxy.sol#L68

=> The owner and stader config in VaultProxy.sol can't be updated anymore and the vault may work with an outdated version of the config and therefore incorrect values. As the withdrawal and ETH distribution logic of both NodeElRewardVault and ValidatorWithdrawalVault rely heavily on the values provided by the stader config, this may result in values being wrongly calculated or funds being sent to the wrong address.

Proof of Concept

The first two tests should pass but will fail because the vault owner is not equal to the admin. The third test should fail but will pass.

pragma solidity 0.8.16;
import 'forge-std/Test.sol';

import '../../contracts/factory/VaultFactory.sol';
import '../../contracts/StaderConfig.sol';
import '@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol';
import '@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol';

contract VaultFactoryTest is Test {
  address admin; // vaultFactory admin + staderConfig admin
  address nodeRegistryContract; // wants to create vault
  address staderConfig;
  VaultFactory vaultFactory;
  bytes32 public constant NODE_REGISTRY_CONTRACT = keccak256('NODE_REGISTRY_CONTRACT');

  function setUp() public {
    admin = vm.addr(100);
    nodeRegistryContract = vm.addr(101);
    // set up stader config
    address ethDepositAddr = vm.addr(102);
    ProxyAdmin proxyAdmin = new ProxyAdmin();
    StaderConfig configImpl = new StaderConfig();
    TransparentUpgradeableProxy configProxy = new TransparentUpgradeableProxy(
      address(configImpl),
      address(proxyAdmin),
      ''
    );
    staderConfig = address(configProxy);
    StaderConfig(staderConfig).initialize(admin, ethDepositAddr);

    // set up vault factory
    VaultFactory factoryImpl = new VaultFactory();
    TransparentUpgradeableProxy factoryProxy = new TransparentUpgradeableProxy(
      address(factoryImpl),
      address(proxyAdmin),
      ''
    );
    vaultFactory = VaultFactory(address(factoryProxy));
    vaultFactory.initialize(admin, staderConfig);

    // grant NODE_REGISTRY_CONTRACT role to nodeRegistryContract
    vm.startPrank(admin);
    vaultFactory.grantRole(NODE_REGISTRY_CONTRACT, nodeRegistryContract);
    vm.stopPrank();
  }

  /**
   * nodeRegistryContract can create a new withdrawVault
   */
  function test_canCreateWithdrawVault() public {
    vm.startPrank(nodeRegistryContract);
    // constructor args: poolId, operatorId, validatorcount, validatorId
    VaultProxy withdrawalVault = VaultProxy(payable(vaultFactory.deployWithdrawVault(1, 11111, 2, 22222)));
    assert(address(withdrawalVault) != address(0));
    // check if initialised correctly
    assert(withdrawalVault.isInitialized() == true);
    assert(withdrawalVault.isValidatorWithdrawalVault() == true);
    assert(withdrawalVault.poolId() == 1);
    assert(withdrawalVault.id() == 22222); // equal to validatorId
    assertEq(address(withdrawalVault.staderConfig()), staderConfig);
    assert(withdrawalVault.vaultSettleStatus() == false);
    // owner is actually the 0 address
    assert(withdrawalVault.owner() == admin);
  }

  /**
   * nodeRegistryContract can create a new NodeELRewardVault
   */
  function test_canCreateNodeELRewardVault() public {
    vm.startPrank(nodeRegistryContract);
    // constructor args: poolId, operatorId
    VaultProxy rewardVault = VaultProxy(payable(vaultFactory.deployNodeELRewardVault(1, 11111)));
    assert(address(rewardVault) != address(0));
    // check if initialised correctly
    assert(rewardVault.isInitialized() == true);
    assert(rewardVault.poolId() == 1);
    assert(rewardVault.id() == 11111); // equal to operatorId
    assertEq(address(rewardVault.staderConfig()), staderConfig);
    assert(rewardVault.vaultSettleStatus() == false);
    // owner is actually the 0 address
    assert(rewardVault.owner() == admin);
  }

  function test_StaderConfigReturns0AsAdmin() public view {
    assert(StaderConfig(staderConfig).getAdmin() == address(0));
  }

  /**
   * The updateAdmin() function correctly adds the new admin to the accountsMap
   */
  function test_afterUpdateCorrectAdminIsReturned() public {
    vm.startPrank(admin);
    // admin updates admin to himself
    StaderConfig(staderConfig).updateAdmin(admin);
    assert(StaderConfig(staderConfig).getAdmin() == admin);
  }
}

Tools Used

Foundry

Recommended Mitigation Steps

This line should be added to the initialize function in StaderConfig.sol

 setAccount(ADMIN, _admin);

Assessed type

Error

Function setVariable should emit event SetVariable instead of SetConstant

The function setVariable in StaderConfig.sol emits the event SetConstant. There is also an event SetVariable defined in the inferface which would be more descriptive of what the function does.

function setVariable(bytes32 key, uint256 val) internal {
  variablesMap[key] = val;
  emit SetConstant(key, val);
}

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

  // Events
    event SetConstant(bytes32 key, uint256 amount);
    event SetVariable(bytes32 key, uint256 amount);

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/interfaces/IStaderConfig.sol#L12-L14

State changes after interactions

The function createLot in Auction.sol does not follow the checks-effects-interactions pattern

// token transfer
if (!IERC20(staderConfig.getStaderToken()).transferFrom(msg.sender, address(this), _sdAmount)) {
            revert SDTransferFailed();
}
// emitting event
emit LotCreated(nextLot, lotItem.sdAmount, lotItem.startBlock, lotItem.endBlock, bidIncrement);
// state changes
nextLot++;

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/Auction.sol#L55-L59

Mismatch between code and documentation

The comments describe the intention of the withdraw function in SDCollateral.sol as follows:

 /// @notice for operator to request withdraw of sd
    /// @dev it does not transfer sd tokens immediately
    /// operator should come back after withdrawal-delay time to claim
    /// this requested sd is subject to slashes

but in reality the sd tokens are transfered immediately and do not have to be claimed.

function withdraw(uint256 _requestedSD) external override {
 ...
  if (!IERC20(staderConfig.getStaderToken()).transfer(payable(operator), _requestedSD)) {
    revert SDTransferFailed();
  }
  ...
}

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/SDCollateral.sol#L54-L73

Missing override keyword

Not technically necessary, but consider adding for consistency https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderConfig.sol#L361 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderConfig.sol#L466 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderConfig.sol#L500-L502 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionlessNodeRegistry.sol#L316

Duplicate event information

Due to the previous check

 if (msg.sender != userRequest.owner) {
            revert CallerNotAuthorizedToRedeem();
        }

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#L174-L176

it is redundant to list both msg.sender and userRequest.owner, both will have the same value.

emit RequestRedeemed(msg.sender, userRequest.owner, etherToTransfer);

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

Empty constructor could be omitted

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/NodeELRewardVault.sol#L14 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/ValidatorWithdrawalVault.sol#L23 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/VaultProxy.sol#L17

Unused imports

INodeRegistry in NodeELRewardvault.sol https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/NodeELRewardVault.sol#L8s IPermissionlessNodeRegistry in SocializingPool.sol https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/SocializingPool.sol#L9 Math.sol is not used in PermissionedPool and PermissionlessPool https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedPool.sol#L22 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionlessPool.sol#L20

Usage of library functions

Library functions are referenced directly throughout the codebase, so the using A for B directive can be omitted.

Example:

selectedPoolCapacity = Math.min(
  Math.min(poolAllocationMaxSize, _newValidatorToRegister),
  Math.min(remainingPoolCapacity, remainingPoolTarget)
);

using Math for uint256;
using SafeMath for uint256;

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PoolSelector.sol#L15-L16 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PoolSelector.sol#L63-L66

Typos / Naming inconsistencies

The spelling "initialise" is used in 3 instances, while the rest of the codebase uses "initialize". https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/VaultProxy.sol#L19 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/VaultProxy.sol#L20 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/factory/VaultFactory.sol#L42

Error OperatorIsDeactivate => OperatorIsDeactivated https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionlessNodeRegistry.sol#L686

 * @dev any one call, permissionless

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