LSD Network - Stakehouse contest - gz627's results

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/SavETHVault.sol#L206-L207 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/SavETHVault.sol#L209

Vulnerability details

Impact

Liquid staking manager call function withdrawETHForStaking(address _smartWallet, uint256 _amount) to withdraw ETH for staking. It's manager's responsibility to set the correct _smartWallet address. However, there is no way to guarantee this. If a typo (or any other reasons) leads to a non-zero non-existent _smartWallet address, this function won't be able to detect the problem, and the ETH transfer statement will always return true. This will result in the ETH permanently locked to a non-existent account.

Proof of Concept

Liquid staking manager call function withdrawETHForStaking(address _smartWallet, uint256 _amount) with a non-zero non-existent _smartWallet address and some _amount of ETH. Function call will succeed but the ETH will be locked to the non-existent _smartWallet address.

Tools Used

Manual audit.

Recommended Mitigation Steps

The problem can be solved if we can verify the _smartWallet is a valid existent smartWallet before ETH transfer. The easiest solution is to verify the smartWallet has a valid owner since the smart wallet we are using is ownable. So, just add the checking owner code before ETH transfer.

[L-01] Un-necessary revert due to typos

In ETH deposit functions, it has a redundant parameter uint256 _amount that has the same value of msg.value. The function also requires that _amount must equal to msg.value, otherwise revert. This will cost more gas and result in transaction failure on user/staker's typos of _amount. Refactoring the relevant functions to cleaner code improves efficiency and saves gas.

Instances (3)

Instance 1: file: contracts/liquid-staking/SavETHVault.sol

Function depositETHForStaking() retactoring:

1. replace Line83 with: function depositETHForStaking(bytes calldata _blsPublicKeyOfKnot) public payable returns (uint256) {
2. replace Line90 with: uint256 _amount = msg.value

Instance 2: file: contracts/liquid-staking/StakingFundsVault.sol

Function depositETHForStaking() refactoring:

1. replace Line113 with function depositETHForStaking(bytes calldata _blsPublicKeyOfKnot) public payable returns (uint256) {
2. replace Line120 with: uint256 _amount = msg.value

Instance 3: file: contracts/liquid-staking/GiantPoolBase.sol#L34 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/GiantPoolBase.sol#L34-L48

1. replace Line34 with function depositETH() public payable {
2. remove Line36

---

[L-02] Repeated business logic should be refactored for easier maintenance and gas saving

file: contracts/liquid-staking/LiquidStakingManager.sol

Function rotateEOARepresentative and rotateEOARepresentativeOfNodeRunner have the same repeated logic that can be refactored as follows(also introducing custom errors to save gas):

    // introduce custom errors for gas saving
    error ZeroAddressRepresentative();
    error InvalidSmartWallet();
    error NotAllKNOTsMinted();
    error InvalidRotationToSameEOA();
    error NodeRunnerIsBanned();

    /// @notice Rotate representative
    /// @param _nodeRunner address of the node runner
    /// @param _newRepresentative address of the new representative to be appointed for the node runner
    function _rotateEOARepresentative(
        address _nodeRunner,
        address _newRepresentative
    ) private {
        if (_newRepresentative == address(0))
            revert ZeroAddressRepresentative();

        address smartWallet = smartWalletOfNodeRunner[_nodeRunner];
        if (smartWallet == address(0)) revert InvalidSmartWallet();
        if (stakedKnotsOfSmartWallet[smartWallet] != 0)
            revert NotAllKNOTsMinted();
        if (smartWalletRepresentative[smartWallet] == _newRepresentative)
            revert InvalidRotationToSameEOA();

        // unauthorize old representative
        _authorizeRepresentative(
            smartWallet,
            smartWalletRepresentative[smartWallet],
            false
        );

        // authorize new representative
        _authorizeRepresentative(smartWallet, _newRepresentative, true);
    }

    /// @notice Allow a node runner to rotate the EOA representative they use for their smart wallet
    /// @dev if any KNOT is staked for a smart wallet, no rep can be appointed or updated until the derivatives are minted
    /// @param _newRepresentative address of the new representative to be appointed
    function rotateEOARepresentative(address _newRepresentative) external {
        if(isNodeRunnerBanned(msg.sender))
          revert NodeRunnerIsBanned();

        _rotateEOARepresentative(msg.sender, _newRepresentative);
    }

    /// @notice Allow DAO to rotate representative in the case that node runner is not available (to facilitate staking)
    /// @param _nodeRunner address of the node runner
    /// @param _newRepresentative address of the new representative to be appointed for the node runner
    function rotateEOARepresentativeOfNodeRunner(
        address _nodeRunner,
        address _newRepresentative
    ) external onlyDAO {
        _rotateEOARepresentative(_nodeRunner, _newRepresentative);
    }

---

[L-03] Inconsistency message leading to confusing

File: contracts/liquid-staking/LiquidStakingManager.sol

Message on Line 940 should be "DAO staking funds vault balance must be 4 ether".

File: contracts/liquid-staking/LiquidStakingManager.sol
940:  require(stakingFundsLP.totalSupply() == 4 ether, "DAO staking funds vault balance must be at least 4 ether");

Further, the above require() statement should be converted to a custom error for gas saving:

error DAOStakingFundsMustBe4Ether();

if (stakingFundsLP.totalSupply() != 4 ether)
    revert DAOStakingFundsMustBe4Ether();

---

[L-04] Seperated business logic of the same function should be merged into one place

In File contracts/liquid-staking/LiquidStakingManager.sol, the business logic of updating DAO revenue commission is spreaded in function _updateDAORevenueCommission and updateDAORevenueCommission. As _updateDAORevenueCommission will be called in different places, it's better to merge the updating logic into one place: _updateDAORevenueCommission.

Refactor _updateDAORevenueCommission() and updateDAORevenueCommission() to the following code (also introducing custom error to save gas):

# File: contracts/liquid-staking/LiquidStakingManager.sol

	 error InvalidCommission();
	 error SameCommissionPercentage();

    /// @dev Internal method for dao to trigger updating commission it takes of node runner revenue
    function _updateDAORevenueCommission(uint256 _commissionPercentage) internal {
        if(_commissionPercentage > MODULO)
    			revert InvalidCommission();
    		if(_commissionPercentage == daoCommissionPercentage)
    			revert SameCommissionPercentage();

        emit DAOCommissionUpdated(daoCommissionPercentage, _commissionPercentage);

        daoCommissionPercentage = _commissionPercentage;
    }

    /// @notice Allow DAO to take a commission of network revenue
    function updateDAORevenueCommission(uint256 _commissionPercentage) external onlyDAO {        
        _updateDAORevenueCommission(_commissionPercentage);
    }

---

[L-05] Redundant statement

file: contracts/liquid-staking/SavETHVault.sol

Line 170 is redundant since dETHDetails is defined as a storage variable at Line 152. Any changes of dETHDetails will be reflected in dETHForKnot[blsPublicKeyOfKnot].

File: contracts/liquid-staking/SavETHVault.sol

170:   dETHForKnot[blsPublicKeyOfKnot] = dETHDetails;

---

[L-06] Add a logging event for receive() function

Instances (4)

.transfer() and .transfer() have a limit of 2300 gas, .call() may pass onto more gas. This amount of gas (2300) is enough for payment and logging but not for some other operations. For best practice, we can add a logging event in the receive() function so that we can receive a notification whenever we receive some ETH.

Instance 1: file: contracts/syndicate/Syndicate.sol receive() function: https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/syndicate/Syndicate.sol#L352-L354

Instance 2: file: contracts/syndicate/LiquidStakingManager.sol receive() function: https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L629

Instance 3: file: contracts/syndicate/SyndicateRewardsProcessor.sol receive() function: https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L98

Instance 4: file: contracts/syndicate/OwnableSmartWallet.sol receive() function: https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/smart-wallet/OwnableSmartWallet.sol#L148

Solution:

Change function receive() to:

    event ReceivedEth(address _from, uint256 _value);
    /// @notice contract can receive ETH
    receive() external payable {
        emit ReceivedEth(msg.sender, msg.value);
    }