Stader Labs - 0xhacksmithh's results

[Low-01] burnFrom() In ETHx.sol contract file gives BURNER_ROLE to burn any accounts Fund

 function burnFrom(address account, uint256 amount) external onlyRole(BURNER_ROLE) whenNotPaused {
        _burn(account, amount); // @audit have power to burn any contracts fund without mentioning them
    }

Instances(1)

File: contracts/ETHx.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/ETHx.sol#L57

[Low-02] Some Functions should be payable which interacting with ETH like transfering ETH to other contracts.

- function withdraw() external override {

+ function withdraw() external override payable{

Instances(5)

File: contracts/NodeELRewardVault.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/NodeELRewardVault.sol#L24

File: contracts/ValidatorWithdrawalVault.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/ValidatorWithdrawalVault.sol#L30
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/ValidatorWithdrawalVault.sol#L54

File: contracts/StaderInsuranceFund.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderInsuranceFund.sol#L41
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderInsuranceFund.sol#L60

[Low-03] ETH could be locked inside a contract

Contract implement payable function like depositFor(), posibility any user can sent eth by mistakely, and as there is no rescue function present in contract, eth will remain locked in that contract

Similarly Another case

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

Similarly Another case

    receive() external payable { 
        emit ETHReceived(msg.sender, msg.value);
    }

Instances(3)

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

File: contracts/OperatorRewardsCollector.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/OperatorRewardsCollector.sol#L40

File: contracts/ValidatorWithdrawalVault.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/ValidatorWithdrawalVault.sol#L26-L28

[Low-04] depositFor() even work when contract is in paused mode

There should be whenNotPaused modifier present

-    function depositFor(address _receiver) external payable { 

+    function depositFor(address _receiver) external payable whenNotPaused { 
        balances[_receiver] += msg.value;

        emit DepositedFor(msg.sender, _receiver, msg.value); // @audit ETH can locked here
    }

Instances(1)

File: contracts/OperatorRewardsCollector.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/OperatorRewardsCollector.sol#L40-L45

[Low-05] Should Check for Zero address before sending Eth

Here UtilLib.getOperatorRewardAddress return receiver address, which getting that via INodeRegistry(nodeRegistry).getOperatorRewardAddress(operatorId)

so this could be 0x0000.. address. So should validate operatorRewardsAddr before calling UtilLib.sendValue(

       .............
       .............

        address operatorRewardsAddr = UtilLib.getOperatorRewardAddress(msg.sender, staderConfig); 
        UtilLib.sendValue(operatorRewardsAddr, amount);
        emit Claimed(operatorRewardsAddr, amount);

Instances(1)

File: contracts/OperatorRewardsCollector.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/OperatorRewardsCollector.sol#L51

[Low-06] Admin can change critical parameter like _staerConfig while contract in paused mode

Adin should not able to do that, at least use some timelock like functionality.

    function updateStaderConfig(address _staderConfig) external onlyRole(DEFAULT_ADMIN_ROLE) { 
        UtilLib.checkNonZeroAddress(_staderConfig);
        staderConfig = IStaderConfig(_staderConfig);
        emit UpdatedStaderConfig(_staderConfig);

Instances(1)

File: contracts/OperatorRewardsCollector.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/OperatorRewardsCollector.sol#L56-L60

[Low-07] addNewPool() Function could be used to create pool with a arbitary _poolId, which could be problematic in upcoming future.

In my opinion _poolId should cached in stack and which could incremented gradually, which could be easily trackable

    function addNewPool(uint8 _poolId, address _poolAddress) external override { 
        UtilLib.onlyManagerRole(msg.sender, staderConfig);
        UtilLib.checkNonZeroAddress(_poolAddress);
        verifyNewPool(_poolId, _poolAddress);
        poolIdArray.push(_poolId);
        poolAddressById[_poolId] = _poolAddress;
        emit PoolAdded(_poolId, _poolAddress);
    }

Instances(1)

File: contracts/PoolUtils.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PoolUtils.sol#L40-L47

[Low-08] While contract receive eth via fallback should emit something

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

Instances(1)

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

[Low-09] Should Check address before making low level to a contract

Here both staderConfig.getValidatorWithdrawalVaultImplementation() and staderConfig.getNodeELRewardVaultImplementation() can return 0x00....., So before making any low level call to these returned address function should ensure that those are not zero address, Otherwise call get successful without revert.

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

Instances(1)

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

[Low-10] nodeRegistry not properly validated here, before making further call to that

   function getUpdatedPenaltyAmount(
        uint8 _poolId,
        uint256 _validatorId,
        IStaderConfig _staderConfig
    ) internal returns (uint256) {
        address nodeRegistry = IPoolUtils(_staderConfig.getPoolUtils()).getNodeRegistry(_poolId); // @audit L::  Not properly validate here
        (, bytes memory pubkey, , , , , , ) = INodeRegistry(nodeRegistry).validatorRegistry(_validatorId);
        bytes[] memory pubkeyArray = new bytes[](1);
        pubkeyArray[0] = pubkey;
        IPenalty(_staderConfig.getPenaltyContract()).updateTotalPenaltyAmount(pubkeyArray);
        return IPenalty(_staderConfig.getPenaltyContract()).totalPenaltyAmount(pubkey);
    }

Instances(1)

File: contracts/ValidatorWithdrawalVault.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/ValidatorWithdrawalVault.sol#L144-L149

[Gas-01] Function for creating lot i.e createLot can be more gas efficient as follows

-        lots[nextLot].startBlock = block.number; // @audit gas efficient way mentioned in RemixId
-        lots[nextLot].endBlock = block.number + duration;
-        lots[nextLot].sdAmount = _sdAmount;

        LotItem storage lotItem = lots[nextLot];

Above could replace with

         LotItem storage lotItem = lots[nextLot]

+        lotItem.startBlock = block.number;
+        lotItem.endBlock = block.number + duration;
+        lotItem.sdAmount = _sdAmount;

First one cost :: 130290 gas Second one cost :: 110009 gas (NB :: I only mentioned here gas used to only store value to state variable)

Instances(1)

File: contracts/Auction.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/Auction.sol#L49-L51

[Gas-02] lotItem variable has no significance in createLot() function, so it should be removed

Instances(1)

File: contracts/Auction.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/Auction.sol#L53

[Gas-03] lotItem.bids[msg.sender] could set to 0 without arithmatic operation due to above check

as withdrawalAmount nothing but lotItem.bids[msg.sender], so lotItem.bids[msg.sender] can directly set to 0

    function withdrawUnselectedBid(uint256 lotId) external override nonReentrant {
        LotItem storage lotItem = lots[lotId];
        if (block.number <= lotItem.endBlock) revert AuctionNotEnded();
        if (msg.sender == lotItem.highestBidder) revert BidWasSuccessful();

        uint256 withdrawalAmount = lotItem.bids[msg.sender];
        if (withdrawalAmount == 0) revert InSufficientETH();

-       lotItem.bids[msg.sender] -= withdrawalAmount;
+       lotItem.bids[msg.sender] = 0;

Instances(1)

File: contracts/Auction.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/Auction.sol#L128

[Gas-04] For transfering ETH use of assembly code can be more gas efficient

-        (bool success, ) = payable(msg.sender).call{value: withdrawalAmount}(''); 
-        if (!success) revert ETHWithdrawFailed();

+        bool succ;
+        assembly{
+            succ := call(gas(), msg.sender, withdrawalAmount, 0, 0)
+        }

Instances(1)

File: contracts/Auction.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/Auction.sol#L131-L132

[Gas-05] Zero address can perform in a function, no need for extra JUMP (i.e for checking just zero address function calling another contract function) which is more gas costlier

You can simply perform zero address check in updateStaderConfig() function, even with assembly this will be more gas efficient, no need to call external contract.

    function updateStaderConfig(address _staderConfig) external onlyRole(DEFAULT_ADMIN_ROLE) {
+        require(_staderConfig != address(0), 'BOOM BOOM');
-        UtilLib.checkNonZeroAddress(_staderConfig); 
         staderConfig = IStaderConfig(_staderConfig);
         emit UpdatedStaderConfig(_staderConfig);
    }

Instances(1)

File: contracts/ETHx.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/ETHx.sol#L86

[Gas-06] Instead of address(this).balance, self.balance() should be used, which is more gas efficient

Instances(7)

File: contracts/NodeELRewardVault.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/NodeELRewardVault.sol#L28

File: contracts/ValidatorWithdrawalVault.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/ValidatorWithdrawalVault.sol#L34
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/ValidatorWithdrawalVault.sol#L99

File: contracts/StaderInsuranceFund.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderInsuranceFund.sol#L43
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderInsuranceFund.sol#L62

[Gas-07] No need to cache msg.sender below

    function claim() external whenNotPaused {
-        address operator = msg.sender; // @audit G=> can optimized to one line.
-        uint256 amount = balances[operator];
-        balances[operator] -= amount;

-        uint256 amount = balances[msg.sender];
-        balances[msg.sender] -= amount;
....
....

Instances(1)

File: contracts/OperatorRewardsCollector.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/OperatorRewardsCollector.sol#L46-L49

[Gas-08] uint storage overhead

uint16 public poolAllocationMaxSize;

mapping(uint8 => uint256) public poolWeights;

Instances(4)

File:: contracts/PoolSelector.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PoolSelector.sol#L18
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PoolSelector.sol#L23

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

[Gas-09] address nodeRegistry should create outside of the loop and get overrided with very iteration of the loop, which will be more gas efficient.

    function isExistingPubkey(bytes calldata _pubkey) public view override returns (bool) { 
        uint256 poolCount = getPoolCount();
        for (uint256 i = 0; i < poolCount; i++) {
            address nodeRegistry = getNodeRegistry(poolIdArray[i]);
            if (INodeRegistry(nodeRegistry).isExistingPubkey(_pubkey)) {
                return true;
            }
        }
        return false;
    }

Instances(4)

File:: contracts/PoolUtils.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PoolUtils.sol#L169
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PoolUtils.sol#L180
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PoolUtils.sol#L191
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PoolUtils.sol#L202

[Gas-10] 4 functions could be enclosed to 1 function with 4 return value

As isExistingPubkey(), isExistingOperator(), getOperatorPoolId() and getValidatorPoolId working, validating and calling same external contract similar manner so these could arranged in a single function with 4 return value which will save some deployment gas cost.

    function isExistingPubkey(bytes calldata _pubkey) public view override returns (bool) { // @audit G:: These 4 function can perform in one function with 4 return value
        uint256 poolCount = getPoolCount();
        for (uint256 i = 0; i < poolCount; i++) {
            address nodeRegistry = getNodeRegistry(poolIdArray[i]); // @audit G:: This variable should created outside of the loop
            if (INodeRegistry(nodeRegistry).isExistingPubkey(_pubkey)) {
                return true;
            }
        }
        return false;
    }

    function isExistingOperator(address _operAddr) external view override returns (bool) {
        uint256 poolCount = getPoolCount();
        for (uint256 i = 0; i < poolCount; i++) {
            address nodeRegistry = getNodeRegistry(poolIdArray[i]); // @audit G:: This variable should created outside of the loop
            if (INodeRegistry(nodeRegistry).isExistingOperator(_operAddr)) {
                return true;
            }
        }
        return false;
    }

    function getOperatorPoolId(address _operAddr) external view override returns (uint8) {
        uint256 poolCount = getPoolCount();
        for (uint256 i = 0; i < poolCount; i++) {
            address nodeRegistry = getNodeRegistry(poolIdArray[i]);  // @audit G:: This variable should created outside of the loop
            if (INodeRegistry(nodeRegistry).isExistingOperator(_operAddr)) {
                return poolIdArray[i];
            }
        }
        revert OperatorIsNotOnboarded();
    }

    function getValidatorPoolId(bytes calldata _pubkey) external view override returns (uint8) {
        uint256 poolCount = getPoolCount();
        for (uint256 i = 0; i < poolCount; i++) {
            address nodeRegistry = getNodeRegistry(poolIdArray[i]);  // @audit G:: This variable should created outside of the loop
            if (INodeRegistry(nodeRegistry).isExistingPubkey(_pubkey)) {
                return poolIdArray[i];
            }
        }
        revert PubkeyDoesNotExit();
    }

Instances(1)

File:: contracts/PoolUtils.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PoolUtils.sol#L166-L209

[Gas-11] Inherite Openzeppelin Initializable library which will be more gas efficient than your custom one

...........
...........
   bool public override isInitialized;
...........
...........
   function initialise( 
        bool _isValidatorWithdrawalVault,
        uint8 _poolId,
        uint256 _id,
        address _staderConfig
    ) external {
        if (isInitialized) {
            revert AlreadyInitialized();
        }
............
............       

Instances(1)

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

[Gas-12] Openzeppelin's safeTransferFrom() should used which is more gas efficient and give more accurate error.

if (!IERC20(staderConfig.getStaderToken()).transferFrom(msg.sender, address(this), _sdAmount)) { 
            revert SDTransferFailed();
        }

Instances(1)

File: contracts/Auction.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/Auction.sol#L55

[Gas-13] address(this) can be stored in state variable that will ultimately cost less, than every time calculating this, specifically when it used multiple times in a contract

Instances(14)

File: contracts/Auction.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/Auction.sol#L55

file: contracts/ValidatorWithdrawalVault.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/ValidatorWithdrawalVault.sol#L31
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/ValidatorWithdrawalVault.sol#L32
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/ValidatorWithdrawalVault.sol#L33
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/ValidatorWithdrawalVault.sol#L34
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/ValidatorWithdrawalVault.sol#L55
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/ValidatorWithdrawalVault.sol#L56
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/ValidatorWithdrawalVault.sol#L57
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/ValidatorWithdrawalVault.sol#L94
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/ValidatorWithdrawalVault.sol#L95

File: contracts/NodeELRewardVault.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/NodeELRewardVault.sol#L25
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/NodeELRewardVault.sol#L26
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/NodeELRewardVault.sol#L27
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/NodeELRewardVault.sol#L28

[Gas-14] Writing to State variable could be more gas efficient with assembly code

Instances(6)

File: contracts/Auction.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/Auction.sol#L144-L149
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/Auction.sol#L151-L155

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

File: StaderConfig.sol
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderConfig.sol#L476-L480
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderConfig.sol#L481-L485