Unrestricted Asset Approval in NodeDelegator.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/NodeDelegator.sol#L35-L46
https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/NodeDelegator.sol#L44
https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/NodeDelegator.sol#L45

Vulnerability details

Impact

The maxApproveToEigenStrategyManager function grants unlimited approval to an external contract to transfer assets from the NodeDelegator contract. This could allow loss of funds if the external contract is compromised.

Proof of Concept

maxApproveToEigenStrategyManager approves max amount of asset to external contract. No way to revoke allowance, so strategy manager contract can potentially drain assets.

The key issue is that it grants unlimited approval to the EigenStrategyManager external contract to transfer assets:

    /// @notice Approves the maximum amount of an asset to the eigen strategy manager
    /// @dev only supported assets can be deposited and only called by the LRT manager
    /// @param asset the asset to deposit
    function maxApproveToEigenStrategyManager(address asset)
        external
        override
        onlySupportedAsset(asset)
        onlyLRTManager
    {
        address eigenlayerStrategyManagerAddress = lrtConfig.getContract(LRTConstants.EIGEN_STRATEGY_MANAGER);
        IERC20(asset).approve(eigenlayerStrategyManagerAddress, type(uint256).max);
    }

The problems with this are:

• There is no way to revoke or reset this unlimited allowance later

• The external EigenStrategyManager could potentially drain assets from the NodeDelegator by transferring them to itself

• If the EigenStrategyManager contract is compromised, the attacker could drain assets

The maxApproveToEigenStrategyManager function approves the maximum uint256 amount of an asset to the EigenStrategyManager address.

• It fetches the EigenStrategyManager address from the LRTConfig contract: L44

address eigenlayerStrategyManagerAddress = 
  lrtConfig.getContract(LRTConstants.EIGEN_STRATEGY_MANAGER);

• Then approves the max amount: L45

IERC20(asset).approve(
  eigenlayerStrategyManagerAddress, 
  type(uint256).max
);

• There are access control checks, but no other limitations on the approval amount. once unlimited approval is granted to the external EigenStrategyManager contract, there is no way to revoke it.

The Issue

The maxApproveToEigenStrategyManager function in NodeDelegator provides unlimited approval to the external EigenStrategyManager contract, with no way to revoke it. This could allow the EigenStrategyManager to drain assets if compromised.

Impact

If the EigenStrategyManager contract is compromised, the attacker could:

• Drain all assets from the NodeDelegator by transferring them to themselves
• Manipulate integrations with the EigenLayer protocol
• Cause loss of deposited funds

Trigger

This could be triggered by:

• Finding an exploit in the EigenStrategyManager contract.
• Separate vulnerability that allows taking over the EigenStrategyManager contract

Maximum impact

• Function approves external contract to transfer unlimited amounts

• External contract has balance of 1000 DAI

• If compromised, attacker can drain all 1000 DAI

• Maximum loss is full balance of given asset approved

A step-by-step explanation from root cause to impact

1. NodeDelegator has 1000 DAI

2. Attacker compromises external contract

3. NodeDelegator calls maxApproveToEigenStrategyManager(DAI)

4. Unlimited DAI approval granted to compromised contract

5. Attacker uses external contract to transfer 1000 DAI to themselves

6. Attacker has drained all 1000 DAI from NodeDelegator

Tools Used

Manual review

Recommended Mitigation Steps

• Set a maximum approval amount cap
• Build a revokeApproval function
• Use increaseAllowance and decreaseAllowance instead
• Validate EigenStrategyManager address being approved

Assessed type

ETH-Transfer

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Intended design callable only by the manager.

[fatherGoose1]

Agree that the max approval without ability to rescind approval adds risk, but the risk is accepted by Kelp. QA

[c4-judge]

fatherGoose1 changed the severity to QA (Quality Assurance)

[c4-judge]

fatherGoose1 marked the issue as grade-b