Kelp DAO | rsETH - jasonxiale's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTOracle.sol#L52-L79

Vulnerability details

Impact

LRTOracle.getRSETHPrice is calculated as spot price, and it can be manipulated.

Proof of Concept

LRTOracle.getRSETHPrice is defined as

    function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        address rsETHTokenAddress = lrtConfig.rsETH();
        uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply();

        if (rsEthSupply == 0) {
            return 1 ether;
        }

        uint256 totalETHInPool;
        address lrtDepositPoolAddr = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);

        address[] memory supportedAssets = lrtConfig.getSupportedAssetList();
        uint256 supportedAssetCount = supportedAssets.length;

        for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
            address asset = supportedAssets[asset_idx];
            uint256 assetER = getAssetPrice(asset);

            uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
            totalETHInPool += totalAssetAmt * assetER;

            unchecked {
                ++asset_idx;
            }
        }

        return totalETHInPool / rsEthSupply;
    }

So the price is calculated as totalETHInPool / rsEthSupply, both totalETHInPool and rsEthSupply can be manipulated.

1. for totalETHInPool can be calculated as: totalETHInPool += totalAssetAmt * assetER. assetER is queried by price feed, which can't be manipulated. But for totalAssetAmt, it's calculated in LRTDepositPool.getTotalAssetDeposits, and LRTDepositPool.getTotalAssetDeposits calls LRTDepositPool.getAssetDistributionData, in LRTDepositPool.getAssetDistributionData, assetLyingInDepositPool and assetLyingInNDCs are calculated by balaceOf

    function getAssetDistributionData(address asset)
        public
        view
        override
        onlySupportedAsset(asset)
        returns (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer)
    {
        // Question: is here the right place to have this? Could it be in LRTConfig?
        assetLyingInDepositPool = IERC20(asset).balanceOf(address(this)); <<<--- Here using balaceOf

        uint256 ndcsCount = nodeDelegatorQueue.length;
        for (uint256 i; i < ndcsCount;) {
            assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]); <<<--- Here using balaceOf
            assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset);
            unchecked {
                ++i;
            }
        }
    }

So if we transfer asset to LRTDepositPool.sol or nodeDelegatorQueue, we can increase totalETHInPool

2. for rsEthSupply, it's the value of IRSETH(rsETHTokenAddress).totalSupply(). And it can be manipulated by RSETH.mint and RSETH.burnFrom

One way to exploit this vulnerable, we can abuse the trick similar to ERC4626 inflation attacks:

1. before all LRTDepositPool.depositAsset tx, the attacker cals LRTDepositPool.depositAsset to deposit 1 wei asset
2. while LRTOracle.getRSETHPrice is called during the tx, since the attacker is the first one to deposit, rsEthSupply will equal to 0, and 1 ether will be returned as price.
3. everything left goes as expected, and the attacker get his rsETH token.
4. then the attacker transfer a large amount of asset to contract LRTDepositPool to increase assetLyingInDepositPool, or transfer a large amount of asset to nodeDelegatorQueue[i] to increase assetLyingInNDCs
5. as the result of previous step, totalETHInPool will be increase, which causes the final price to increase
6. when Alice(the honest user) try to call LRTDepositPool.depositAsset, while calculating the amount of rsETH she can get, she'll get less than expected because of the high price

Tools Used

VIM

Recommended Mitigation Steps

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTConfig.sol#L109-L122

Vulnerability details

Impact

strategy is used to deposit asset as shown NodeDelegator.depositAssetIntoStrategy. When calculating the rsETH price in LRTOracle.getRSETHPrice, the assets deposited in strategies will be used to calculate totalETHInPool. Thus if replacing a existing strategy containing deposited asset will cause the price decline

Proof of Concept

In LRTConfig.updateAssetStrategy, before replacing an existing strategy, the function doesn't check if the strategy has asset deposited in.

    function updateAssetStrategy(
        address asset,
        address strategy
    )
        external
        onlyRole(DEFAULT_ADMIN_ROLE)
        onlySupportedAsset(asset)               <<<<------------- check if the protocol support the asset
    {
        UtilLib.checkNonZeroAddress(strategy);  <<<------- check if the strategy's address is 0
        if (assetStrategy[asset] == strategy) { <<<------- check if the new strategy is the same as current 
            revert ValueAlreadyInUse();
        }
        assetStrategy[asset] = strategy;
    }

When a user calls LRTDepositPool.depositAsset to deposit some stETH, within the function, LRTDepositPool._mintRsETH is called. And the amount of rsETH to be minted for the user is calculated in LRTDepositPool.sol#L109 as rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();

While calling LRTOracle.getRSETHPrice, the function will add up all ETH in every LRTDepositPool

So the code flow will go to LRTDepositPool.getTotalAssetDeposits, and next will go to LRTDepositPool.getAssetDistributionData.

Within LRTDepositPool.getAssetDistributionData, the function will call NodeDelegator.getAssetBalance. In NodeDelegator.getAssetBalance, the function will requery the amount of underlyingToken

    function getAssetBalance(address asset) external view override returns (uint256) {
        address strategy = lrtConfig.assetStrategy(asset);
        return IStrategy(strategy).userUnderlyingView(address(this));
    }

So the if the admin calls LRTConfig.updateAssetStrategy without checking the underlying balance, the price might be incorrectly.

Tools Used

VIM

Recommended Mitigation Steps

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L162-L176

Vulnerability details

Impact

LRTDepositPool.addNodeDelegatorContractToQueue lacks of the duplicated nodes existing in the LRTDepositPool.nodeDelegatorQueue, if there are duplicated nodes, it will increase price when calling LRTOracle.getRSETHPrice to calculating the price.

Proof of Concept

In LRTDepositPool.addNodeDelegatorContractToQueue, there is no check if there are duplicated nodes existing in LRTDepositPool.nodeDelegatorQueue before push the new node into the queue.

    function addNodeDelegatorContractToQueue(address[] calldata nodeDelegatorContracts) external onlyLRTAdmin {
        uint256 length = nodeDelegatorContracts.length;
        if (nodeDelegatorQueue.length + length > maxNodeDelegatorCount) {
            revert MaximumNodeDelegatorCountReached();
        }

        for (uint256 i; i < length;) {
            UtilLib.checkNonZeroAddress(nodeDelegatorContracts[i]);
            nodeDelegatorQueue.push(nodeDelegatorContracts[i]);
            emit NodeDelegatorAddedinQueue(nodeDelegatorContracts[i]);
            unchecked {
                ++i;
            }
        }
    }

When calling LRTOracle.getRSETHPrice, ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits will be called, and in LRTDepositPool.getTotalAssetDeposits, LRTDepositPool.getAssetDistributionData will be called. If duplicated nodes existing, the duplicated node will be calculated twice:

    function getAssetDistributionData(address asset)
        public
        view
        override
        onlySupportedAsset(asset)
        returns (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer)
    {
        // Question: is here the right place to have this? Could it be in LRTConfig?
        assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));

        uint256 ndcsCount = nodeDelegatorQueue.length;
        for (uint256 i; i < ndcsCount;) {
            assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]);
            assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset);
            unchecked {
                ++i;
            }
        }

The following test can prove that duplicated elements can be added, add the following code in LRTDepositPoolTest.t.sol, under LRTDepositPoolGetNodeDelegatorQueue contract, and run forge test --mc LRTDepositPoolGetNodeDelegatorQueue --mt test_GetNodeDelegatorQueueDup

    function test_GetNodeDelegatorQueueDup() external {
        address nodeDelegatorContractOne = address(new MockNodeDelegator());
        address nodeDelegatorContractTwo = address(new MockNodeDelegator());
        address nodeDelegatorContractThree = address(new MockNodeDelegator());

        address[] memory nodeDelegatorQueue = new address[](4);
        nodeDelegatorQueue[0] = nodeDelegatorContractOne;
        nodeDelegatorQueue[1] = nodeDelegatorContractTwo;
        nodeDelegatorQueue[2] = nodeDelegatorContractThree;
        nodeDelegatorQueue[3] = nodeDelegatorContractTwo;

        vm.startPrank(admin);
        lrtDepositPool.addNodeDelegatorContractToQueue(nodeDelegatorQueue);
        vm.stopPrank();

        assertEq(lrtDepositPool.getNodeDelegatorQueue(), nodeDelegatorQueue, "Node delegator queue is not set");
    }

Tools Used

VIM

Recommended Mitigation Steps

Assessed type

Other