Approvals are not revoked when changing the strategy

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Even if you remove a strategy, it can still control NodeDelegator funds due to an oversight in approvals not being revoked.

Proof of Concept

maxApproveToEigenStrategyManager() is used to approve the maximum amount for EIGEN_STRATEGY_MANAGER:

    function maxApproveToEigenStrategyManager(address asset)
        external
        override
        onlySupportedAsset(asset)
        onlyLRTManager
    {
        address eigenlayerStrategyManagerAddress = lrtConfig.getContract(LRTConstants.EIGEN_STRATEGY_MANAGER);
        IERC20(asset).approve(eigenlayerStrategyManagerAddress, type(uint256).max);
    }

However, changing EIGEN_STRATEGY_MANAGER in LRTConfig can lead to issues, as old strategies may still control NodeDelegator funds. This poses a security risk if an attacker compromises an old strategy.

    function setContract(bytes32 contractKey, address contractAddress) external onlyRole(DEFAULT_ADMIN_ROLE) {
        _setContract(contractKey, contractAddress);
    }

Therefore, old strategy's will still be able to manage NodeDelegator balance, this could be a problem if the old strategy was compromised by an attacker.

Tools Used

Manual review

Recommended Mitigation Steps

Revoke the old approvals when a new strategy is set.

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