Kelp DAO | rsETH - MaslarovK's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L183-L197 https://github.com/code-423n4/2023-11-kelp/blob/main/src/NodeDelegator.sol#L74-L89

Vulnerability details

Impact

The function attempts to transfer a specified amount of an asset to a contract. However, there is no check to ensure that the current contract has sufficient balance of the asset before attempting the transfer. If the specified amount is greater than the contract's current balance of the asset, the transfer will fail and the entire transaction will revert. Even though there is a check which reverts if the transfer fails, the transaction will revert before this, when the external call fails due to insufficient amount of tokens and the gas used to this moment will still be consumed.

Proof of Concept

I made slight change in the test below which shows the possibility of transfer attempt with amount bigger than the current balance:

function test_TransferAssetToNodeDelegator() external {
        // deposit 3 ether rETH
        vm.startPrank(alice);
        rETH.approve(address(lrtDepositPool), 3 ether);
        lrtDepositPool.depositAsset(address(rETH), 3 ether);
        vm.stopPrank();

        uint256 indexOfNodeDelegatorContractOneInNDArray;
        address[] memory nodeDelegatorArray = lrtDepositPool.getNodeDelegatorQueue();
        for (uint256 i = 0; i < nodeDelegatorArray.length; i++) {
            if (lrtDepositPool.nodeDelegatorQueue(i) == nodeDelegatorContractOne) {
                indexOfNodeDelegatorContractOneInNDArray = i;
                break;
            }
        }

        // transfer 4 ether rETH to node delegator contract one
        vm.startPrank(manager);
        lrtDepositPool.transferAssetToNodeDelegator(indexOfNodeDelegatorContractOneInNDArray, address(rETH), 4 ether);
        vm.stopPrank();

        assertEq(rETH.balanceOf(address(lrtDepositPool)), 2 ether, "Asset amount in lrtDepositPool is incorrect");
        assertEq(rETH.balanceOf(nodeDelegatorContractOne), 1 ether, "Asset is not transferred to node delegator");
    }

resulting in this: [FAIL. Reason: ERC20: transfer amount exceeds balance] test_TransferAssetToNodeDelegator() (gas: 232868)

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend to add the following check to ensure that the contract has sufficient balance before attempting the transfer.

uint256 balance = IERC20(asset).balanceOf(address(this));
require(balance >= amount, "Insufficient balance");

Assessed type

Token-Transfer