code-423n4 / 2023-11-kelp-findings

13 stars 11 forks source link

LRTConfig.updateAssetStrategy() fails to withdraw funds from the old strategy before setting a new strategy for the asset. #638

Closed c4-submissions closed 11 months ago

c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTConfig.sol#L109-L122

Vulnerability details

Impact

Detailed description of the impact of this finding. LRTConfig.updateAssetStrategy() fails to withdraw funds from the old strategy before setting a new strategy for the asset. There are two impacts for this: 1) The funds in the old strategy might be lost; 2) the total value for the asset returned by getTotalAssetDeposits() will be wrong since it does not include the portion in the old strategy; 3) Then the LRTOracle.getRSETHPrice() will return a smaller value too. Such price drop might lead trading that take advantage of the situation.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

LRTConfig.updateAssetStrategy() allows the admin to change the strategy for an asset to a new one.

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTConfig.sol#L109-L122

However, the function fails to withdraw the funding from the old strategy first and as a result those funding might be lost. In addition, the following impacts will ensure: 1) the total value for the asset returned by getTotalAssetDeposits() will be wrong since it does not include the portion in the old strategy; and 2) Then the LRTOracle.getRSETHPrice() will return a smaller value too. Such price drop might lead trading that take advantage of the situation.

Tools Used

VSCode

Recommended Mitigation Steps

Make sure the balance for the strategy is zero before changing an old strategy to a new one.

Assessed type

Invalid Validation

c4-pre-sort commented 11 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 11 months ago

raymondfam marked the issue as duplicate of #197

c4-judge commented 10 months ago

fatherGoose1 marked the issue as unsatisfactory: Invalid

c4-judge commented 10 months ago

fatherGoose1 changed the severity to 2 (Med Risk)

c4-judge commented 10 months ago

fatherGoose1 marked the issue as satisfactory