Unnecessary max approve puts node delegator assets at risk

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/NodeDelegator.sol#L45

Vulnerability details

Impact

In NodeDelegator.sol, the only method to deposit assets into an EigenLayer Strategy requires the MANAGER role to call maxApproveToEigenStrategyManager(address) which approves the maximum allowance of an asset to an EigenLayer strategy. This allowance stays at the maximum and cannot be updated or revoked.

https://github.com/code-423n4/2023-11-kelp/blob/main/src/NodeDelegator.sol#L45

If any vulnerability is found in a particular EigenLayer strategy that takes advantage of this allowance, all assets deposited to a node delegator are at risk.

This is medium severity due to having an external requirement in EigenLayer.

Proof of Concept

1. Admin adds a node delegator https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L162
2. MANAGER of NodeDelegator calls maxApproveToEigenStrategyManager(address) https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L162
3. Configured EIGEN_STRATEGY_MANAGER contract has full allowance of asset held by NodeDelegator. A vulnerability in this contract could put all assets at risk

Tools Used

Manual review

Recommended Mitigation Steps

A potential mitigation is to only approve the amount needed at the time of deposit.

Current:
https://github.com/code-423n4/2023-11-kelp/blob/main/src/NodeDelegator.sol#L67

function depositAssetIntoStrategy(address asset)
        external
        override
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
        onlyLRTManager
    {
        ...
        IEigenStrategyManager(eigenlayerStrategyManagerAddress).depositIntoStrategy(IStrategy(strategy), token, balance);
    }

Recommended:

```solidity
function depositAssetIntoStrategy(address asset)
        external
        override
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
        onlyLRTManager
    {
        ...
        token.approve(eigenlayerStrategyManagerAddress, balance);
        IEigenStrategyManager(eigenlayerStrategyManagerAddress).depositIntoStrategy(IStrategy(strategy), token, balance);
    }

Assessed type

ERC20

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #70

[c4-judge]

fatherGoose1 changed the severity to QA (Quality Assurance)

[c4-judge]

fatherGoose1 marked the issue as grade-b