Kelp DAO | rsETH - Juntao's results

Lines of code

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

Vulnerability details

Impact

Depositor may be grief attacked and no rsETH will be minted.

Proof of Concept

Depositor can mint rsETH by depositing asset through depositAsset(...) function. Asset will be transferred from depositor to the protocol:

        if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
            revert TokenTransferFailed();
        }

Then rsETH will be minted through _mintRsETH(...) function:

        uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

The amount of rsETH to be minted is calculated is determined by rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice(), here lrtOracle.getRSETHPrice() returns the rsETH price:

        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;

As we can see from above, the rsETH price is determined by the asset owned by the protocol and the total supply of rsETH. However, as the asset will be transferred to protocol before minting, the depositor may not receive a correct amount of rsETH, and even worse, depositor can be grief attacked and receive no rsETH at all. Imagine the following scenario:

1. Alice submits a transaction to mint rsETH by depositing 1 ether stETH (price is 1e18);
2. Bob front-runs Alice's transaction and deposits 1 wei stETH;
3. Because Bob is the first depositor, 1 wei rsETH will be minted to Bob, so totalETHInPool is 1 and rsEthSupply is 1;
4. Alice's transaction is executing, because 1 ether is transferred before minting, so totalETHInPool is 1e18 + 1 and rsEthSupply is 1, rsETH price is 1e18 * (1e18 + 1);
5. rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice() = (1e18 * 1e18) / (1e18 * (1e18 + 1)) = 0;
6. Alice is thus grief attacked and received no rsETH.

Please see below test case:

function test_AuditDepositAsset() external {
        ProxyAdmin proxyAdmin = new ProxyAdmin();
        LRTOracle lrtOracleImpl = new LRTOracle();
        TransparentUpgradeableProxy lrtOracleProxy = new TransparentUpgradeableProxy(
            address(lrtOracleImpl),
            address(proxyAdmin),
            ""
        );
        LRTOracle lrtOracle = LRTOracle(address(lrtOracleProxy));
        lrtOracle.initialize(address(lrtConfig));

        vm.startPrank(manager);
        lrtOracle.updatePriceOracleFor(address(stETH), address(new MockPriceOracle()));
        lrtOracle.updatePriceOracleFor(address(cbETH), address(new MockPriceOracle()));
        lrtOracle.updatePriceOracleFor(address(rETH), address(new MockPriceOracle()));
        vm.stopPrank();

        vm.startPrank(admin);
        lrtConfig.setContract(LRTConstants.LRT_DEPOSIT_POOL, address(lrtDepositPool));
        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(lrtOracle));
        vm.stopPrank();


        vm.startPrank(bob);
        rETH.approve(address(lrtDepositPool), 1);
        lrtDepositPool.depositAsset(rETHAddress, 1);
        uint256 bobBalance = rseth.balanceOf(address(bob));
        console.log(bobBalance);
        vm.stopPrank();

        vm.startPrank(alice);

        rETH.approve(address(lrtDepositPool), 1 ether);
        // lrtDepositPool.depositAsset(rETHAddress, 2 ether);
        lrtDepositPool.depositAsset(rETHAddress, 1 ether);

        uint256 aliceBalance = rseth.balanceOf(address(alice));
        assertEq(aliceBalance, 0);

        vm.stopPrank();
    }

Tools Used

Manual Review

Recommended Mitigation Steps

When depositing, rsETH should be minted before transferring asset.

+       // interactions
+       uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

        if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
            revert TokenTransferFailed();
        }


-       // interactions
-       uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/NodeDelegator.sol#L121-L124

Vulnerability details

Impact

Asset staked EigenLayer is not properly calculated, this could lead to incorrect total asset balance and incorrect rsETH price.

Proof of Concept

To get the asset balance staked in EigenLayer, protocol calls userUserlyingView(...) function o get the asset balance staked in EigenLayer, this function is defined in StrategyBase contract as below:

    function userUnderlyingView(address user) external view virtual returns (uint256) {
        return sharesToUnderlyingView(shares(user));
    }

Where [shares(...)]https://etherscan.io/address/0xdfda04f980be6a64e3607c95ca26012ab9aa46d3#code#F2#L267) function is called to get user's share:

    function shares(address user) public view virtual returns (uint256) {
        return strategyManager.stakerStrategyShares(user, IStrategy(address(this)));
    }

stakerStrategyShares is a mapping define in StrategyManagerStorage contract, when user deposits into strategy, share will be updated accordingly:

        _addShares(depositor, strategy, shares);

When user withdraws, it's reasonable to think user's share will be burned and asset will be returned immediately, however, this is not exactly true. To withdraw from EigenLayer, queueWithdrawal(...) function should be firstly called, user's share will be decreased:

            if (_removeShares(msg.sender, strategyIndexes[strategyIndexIndex], strategies[i], shares[i])) {

Then completeQueuedWithdrawal(...) is required to complete the withdrawal and the asset will be returned to user. It is unlikely these 2 functions can be called in a single transaction, as said in the comment:

* @dev middlewareTimesIndex should be calculated off chain before calling this function by finding the first index that satisfies `slasher.canWithdraw`

So the problem is that the protocol does not take the queued withdrawal asset amount into consideration, incorrect total asset balance will be retrieved, if a user deposits before withdrawals are completed, incorrect rsETH price will be used to calculated the rsETH token to be minted, results in user receiving incorrect amount of rsETH. Withdrawal function is a must to the protocol even if it has not been implemented, incorrect calculation of asset balance should be fixed in the current release to prevent the issue mention above.

Tools Used

Manual Review

Recommended Mitigation Steps

When calculating the asset balance staked into EigenLayer, the queued withdrawal balance should be took into consideration, protocol can save the queued balance in local and use it in the calculation.

Assessed type

Access Control