search

code-423n4 / 2023-08-goodentry-findings

3 stars 2 forks source link

All withdrawals after the first one will burn users' liquidity for nothing when the pool is not enabled in `GeVault::withdraw()` #321

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/GeVault.sol#L214-L241 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/GeVault.sol#L321-L333 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/GeVault.sol#L392-L398 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/GeVault.sol#L420-L424

Vulnerability details

GeVault doesn't allow depositing to the pool when the pool is not enabled but it allows withdrawing anytime, even when the pool is disabled. The first withdrawal transaction during the disabled period will remove all assets from the ticks but these assets will not be deployed again. The assumption was that the assets will stay in the vault until everyone withdraws their underlying token, and it is assumed that the users can withdraw their assets from the vault. You can see it in the developer's comment here:

//if pool enabled, deploy assets in ticks, otherwise just let assets sit here until totally withdrawn

But the other users who call the withdraw() function during the disabled period after the first withdrawal can not withdraw their assets, instead, their liquidity will be burned and they will get nothing in return.

Now, let's examine the withdraw() function:
https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/GeVault.sol#L209-L241

file: GeVault.sol
  /// @dev For simplicity+efficieny, withdrawal is like a rebalancing, but a subset of the tokens are sent back to the user before redeploying
214.  function withdraw(uint liquidity, address token) public nonReentrant returns (uint amount) {
215.    require(poolMatchesOracle(), "GEV: Oracle Error");
216.    if (liquidity == 0) liquidity = balanceOf(msg.sender);
217.    require(liquidity <= balanceOf(msg.sender), "GEV: Insufficient Balance");
218.    require(liquidity > 0, "GEV: Withdraw Zero");
219.    
220.--> uint vaultValueX8 = getTVL();
221.    uint valueX8 = vaultValueX8 * liquidity / totalSupply();
222.    amount = valueX8 * 10**ERC20(token).decimals() / oracle.getAssetPrice(token);
223.    uint fee = amount * getAdjustedBaseFee(token == address(token1)) / 1e4;
224.    
225.--> _burn(msg.sender, liquidity);
226.--> removeFromAllTicks();
227.    ERC20(token).safeTransfer(treasury, fee);
228.    uint bal = amount - fee;
229.
230.    if (token == address(WETH)){
231.      WETH.withdraw(bal);
232.      payable(msg.sender).transfer(bal);
233.    }
234.    else {
235.      ERC20(token).safeTransfer(msg.sender, bal);
236.    }  
237.  
238.    // if pool enabled, deploy assets in ticks, otherwise just let assets sit here until totally withdrawn
239.--> if (isEnabled) deployAssets();
240.    emit Withdraw(msg.sender, token, amount, liquidity);
241.  }

These will happen after some sanity checks when the function is called:

  1. TVL will be calculated in line 220.

  2. The corresponding value of the user's liquidity will be calculated according to the total supply. Also, the token amount to send and the fee amount will be calculated in lines 221-223.

  3. The user's liquidity will be burned in line 225.

  4. All the assets will be removed from ticks in line 226.

  5. The token transfers will be made

  6. And lastly, all the remaining assets after the token transfer will be deployed again if the pool is enabled. Otherwise, none of the assets will be deployed back to the ticks.

Everything seems fine, but there is a catch. getTVL() function. The root of the vulnerability is the calculation of the TVL in the getTVL(), which uses tick balances. Here is the function:
https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/GeVault.sol#L390-L398

file: GeVault.sol
  /// @notice Calculate the vault total ticks value
  /// @return valueX8 Total value of the vault with 8 decimals
  function getTVL() public view returns (uint valueX8){
    for(uint k=0; k<ticks.length; k++){
      TokenisableRange t = ticks[k];
-->   uint bal = getTickBalance(k);
      valueX8 += bal * t.latestAnswer() / 1e18;
    }
  }

getTVL() will call the getTickBalance() for every tick and will calculate the TVL. Here is the getTickBalance() function:
https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/GeVault.sol#L417C1-L424C4

file: GeVault.sol
  function getTickBalance(uint index) public view returns (uint liquidity) {
    TokenisableRange t = ticks[index];
    address aTokenAddress = lendingPool.getReserveData(address(t)).aTokenAddress;
--> liquidity = ERC20(aTokenAddress).balanceOf(address(this));
  }

Normally,

getTickBalance() and as a result of this, the getTVL() functions calculate the TVL according to the total value of aToken balances. The vault has to deposit into the lending pool to get the aTokens. If the assets are not deposited into the lending pool, the vault will not have aTokens, and the TVL will be zero according to these calculations.

TVL calculation was one half of the issue, now let's check the other half, which is the removeFromAllTicks() call during the withdraw() function in line 226. It will call the removeFromTick() for every tick and let's check this function:
https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/GeVault.sol#L321-L333

file:GeVault.sol
      /// @notice Remove from tick
321.  function removeFromTick(uint index) internal {
322.    TokenisableRange tr = ticks[index];
323.--> address aTokenAddress = lendingPool.getReserveData(address(tr)).aTokenAddress;
324.--> uint aBal = ERC20(aTokenAddress).balanceOf(address(this));
325.    uint sBal = tr.balanceOf(aTokenAddress);
326.
327.    // if there are less tokens available than the balance (because of outstanding debt), withdraw what's available
328.    if (aBal > sBal) aBal = sBal;
329.    if (aBal > 0){
330.-->   lendingPool.withdraw(address(tr), aBal, address(this));
331.-->   tr.withdraw(aBal, 0, 0);
332.    }
333.  }

This function is actually the opposite of the deployAndStash() function.

After removeFromAllTicks() function is executed, no asset is left in the lending pool, all of the aTokens are burnt, and the vault is filled with token0 and token1.

So, after executing the removeFromAllTicks() but before calling the deployAssets(), the calculated TVL is 0. Here comes the question: Is there a situation where all the assets are removed from the ticks, but not deployed again? And yes, indeed there is: Every withdrawal except the first one during the disabled period.

Now, let's check the withdraw() function one last time (Assume the pool is disabled):

file: GeVault.sol
  /// @dev For simplicity+efficieny, withdrawal is like a rebalancing, but a subset of the tokens are sent back to the user before redeploying
214.  function withdraw(uint liquidity, address token) public nonReentrant returns (uint amount) {
215.    require(poolMatchesOracle(), "GEV: Oracle Error");
216.    if (liquidity == 0) liquidity = balanceOf(msg.sender);
217.    require(liquidity <= balanceOf(msg.sender), "GEV: Insufficient Balance");
218.    require(liquidity > 0, "GEV: Withdraw Zero");
219.    
220.--> uint vaultValueX8 = getTVL();
221.    uint valueX8 = vaultValueX8 * liquidity / totalSupply();
222.    amount = valueX8 * 10**ERC20(token).decimals() / oracle.getAssetPrice(token);
223.    uint fee = amount * getAdjustedBaseFee(token == address(token1)) / 1e4;
224.    
225.--> _burn(msg.sender, liquidity);
226.--> removeFromAllTicks();
227.    ERC20(token).safeTransfer(treasury, fee);
228.    uint bal = amount - fee;
229.
230.    if (token == address(WETH)){
231.      WETH.withdraw(bal);
232.      payable(msg.sender).transfer(bal);
233.    }
234.    else {
235.      ERC20(token).safeTransfer(msg.sender, bal);
236.    }  
237.  
238.    // if pool enabled, deploy assets in ticks, otherwise just let assets sit here until totally withdrawn
239.--> if (isEnabled) deployAssets();
240.    emit Withdraw(msg.sender, token, amount, liquidity);
241.  }

  1. The first withdrawer calls the function, everything works fine, TVL is calculated correctly, all assets are removed from ticks, and token transfer is made, but the assets are not deployed again since the pool is disabled.

  2. At this moment, the aToken balances of the vault is zero, and the vault is filled with token0 and token1.

  3. Next withdrawer calls the function.

  4. The getTVL() in line 220 will return 0 since the vault doesn't have any aToken. vaultValueX8 is 0.

  5. This will cause valueX8, amount and fee is also 0 in lines 221-223.

  6. The user liquidity will be burned in line 225.

  7. removeFromAllTicks() will not do anything, and also will not revert.

  8. Zero-value transfers are allowed in ERC20 implementations. Therefore line 227 and line 235 will also not revert (assuming token != address(WETH)).

  9. The user will lose all of his/her liquidity and not get anything in return.

Impact

Users who call withdraw when the pool is not enabled will lose their liquidity (except the first caller).

Proof of Concept

All of detailed explanations are in the section above.

The code snippets: https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/GeVault.sol#L214-L241

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/GeVault.sol#L321-L333

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/GeVault.sol#L392-L398

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/GeVault.sol#L420-L424

Tools Used

Manual review

Recommended Mitigation Steps

The easiest way to mitigate this is not allowing users to withdraw their funds when the pool is disabled.

There might be other options to mitigate this issue but every other option that comes to my mind also breaks another part while fixing this since there are so many moving parts. That's why I would recommend disabling withdrawals too as a short-term solution until finding a better way.

Assessed type

Other

c4-pre-sort commented 1 year ago

141345 marked the issue as duplicate of #223

c4-judge commented 1 year ago

gzeon-c4 marked the issue as satisfactory