search

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

13 stars 11 forks source link

Logic issue to include new deposit amount into the calculation of rsETH price in LRTDepositPool::depositAsset() #164

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/LRTDepositPool.sol#L136-L141 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L79

Vulnerability details

Impact

In LRTDepositPool::depositAsset(), it calculates the rsETH amount to be minted to the depositor per the input asset amount (a), the asset:ETH price (pa) and rsETH:ETH price (pb): (a*pa)/pb.

Per design, the rsETH:ETH price shall be calculated per the current assets balances and the current rsETH supply in the protocol. However, current implementation takes the new deposit amount also into the calculation of the rsETH:ETH price, because the new deposit is transferred into the contract before the calculation of the rsETH:ETH price. As a result, the rsETH:ETH price is a somehow higher than expected and the depositor will get less rsETH minted.

Proof of Concept

  1. In LRTDepositPool::depositAsset(), it pulls the new deposit into the contract first (line 136), then calculates the rsETH amount to be minted to the depositor (line 141).
  2. The new rsETH amount depends on the rsETH:ETH price which is calculated per the total assets in the pool and the rsETH supply. The total assets is retrieved by LRTDepositPool::getTotalAssetDeposits() function. Because we have pulled in the new deposit in step 1, the total assets deposits becomes bigger than expected. As a result, the calculated rsETH:ETH price gets higher than expected and the new rsETH amount gets smaller than expected. Finally, the depositor gets less rsETH.

Tools Used

Recommended Mitigation Steps

In LRTDepositPool::depositAsset(), _mintRsETH() first and pull user deposit at last (i.e., IERC20(asset).transferFrom()).

Assessed type

Timing

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 #62

c4-judge commented 10 months ago

fatherGoose1 marked the issue as satisfactory

c4-judge commented 10 months ago

fatherGoose1 changed the severity to 2 (Med Risk)

c4-judge commented 10 months ago

fatherGoose1 changed the severity to 3 (High Risk)