Kelp DAO | rsETH - seerether's results

Lines of code

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

Vulnerability details

Impact

• Funds at risk: All tokens approved to the external contract could potentially be drained by a malicious contract. This puts user funds at risk of theft. The severity is high: • Critical severity: This approve-max pattern undermines the security model of ERC20 approvals. It gives unlimited access to external contracts, which is a critical security risk.

Proof of Concept

Approving the maximum amount of an ERC20 token to transfer could potentially be dangerous and lead to token theft. Here is a more detailed explanation: The key issue is in this code:

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

This approves the EigenStrategyManager contract to transfer the maximum possible amount of the ERC20 token. The approve() function on ERC20 tokens allows a spender (in this case the EigenStrategyManager) to transfer up to a specified amount from the caller's account. By approving the maximum uint256 amount, it gives the EigenStrategyManager contract full control to transfer any and all of that token from the NodeDelegator's balance. This could be exploited by a malicious EigenStrategyManager contract that drains all the approved tokens into another account. Or if the EigenStrategyManager contract itself is compromised with a vulnerability, an attacker could drain the tokens

In summary, approving the maximum amount gives too much power to the spender contract and could lead to theft.

Tools Used

Manual

Recommended Mitigation Steps

Only approve the exact amount needed for the deposit:

   uint256 depositAmount = IERC20(asset).balanceOf(address(this));
   IERC20(asset).approve(eigenlayerStrategyManagerAddress, depositAmount );

This restricts the approval to only the amount being deposited. Additionally, you could add a call to approve(0) after the deposit to fully revoke the approval:

   IERC20(asset).approve(eigenlayerStrategyManagerAddress, 0);

This prevents any potential issues if the full approval amount was not used.

Assessed type

Other