Good Entry - Jeiwan's results

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/4b785d455fff04629d8675f21ef1d1632749b252/contracts/GeVault.sol#L226 https://github.com/code-423n4/2023-08-goodentry/blob/4b785d455fff04629d8675f21ef1d1632749b252/contracts/GeVault.sol#L239

Vulnerability details

Impact

When withdrawing funds from a disabled vault, users can burn all their LP tokens and receive 0 asset tokens, basically causing a loss of all their previously deposited tokens. This impacts all users who withdraw after rebalance() has been called or after a user has withdrawn their LPs earlier.

Proof of Concept

The GeVault can be paused by the owner by setting the isEnabled flag to true via a call to the GeVault.setEnabled() function. When the vault is paused:

1. deposits are disallowed;
2. withdrawals are allowed, but assets are re-deployed after a withdrawal;
3. rebalancing is allowed, but assets are also no re-deployed.

Since assets are not re-deployed during a withdrawal, the calculation of TVL of the vault and the calculation of the token amount returned to the user during a withdrawal is broken:

1. When burning LP tokens, the current vault's value is used to determine how much asset tokens to return to the user (GeVault.sol#L220-L223):

   uint vaultValueX8 = getTVL();
   uint valueX8 = vaultValueX8 * liquidity / totalSupply();
   amount = valueX8 * 10**ERC20(token).decimals() / oracle.getAssetPrice(token);
   uint fee = amount * getAdjustedBaseFee(token == address(token1)) / 1e4;

2. The getTVL() function calls getTickBalance() on each tick (GeVault.sol#L392):

   for(uint k=0; k<ticks.length; k++){
       TokenisableRange t = ticks[k];
       uint bal = getTickBalance(k);
       valueX8 += bal * t.latestAnswer() / 1e18;
   }

3. The getTickBalance() function gets the aToken balance of the vault (GeVault.sol#L420):

   address aTokenAddress = lendingPool.getReserveData(address(t)).aTokenAddress;
   liquidity = ERC20(aTokenAddress).balanceOf(address(this));

4. However, the aToken balance will be 0, since the removeFromTick() function (which is called in the withdraw function) has already redeemed all aTokens, as well as LP tokens:

   lendingPool.withdraw(address(tr), aBal, address(this));
   tr.withdraw(aBal, 0, 0);

5. Also, the getTVL() function calls the TokenisableRange.latestAnswer() function to get the price of 1 LP token:

   TokenisableRange t = ticks[k];
   uint bal = getTickBalance(k);
   valueX8 += bal * t.latestAnswer() / 1e18;

6. The latestAnswer() function computes the value of the total liquidity deposited to the underlying pool and divides it by the number of LP tokens issued, to compute per-LP-token value (TokenisableRange.sol#L338).
7. However, since liquidity has already been removed from all ticks, latestAnswer() will also return 0.

As a result, the getTVL() call will return 0, which will result in 0 token amounts returned to the user. However, all user's LP tokens will be burned.

The issue doesn't impact the first user who withdraws from a paused vault–it only impact those who withdraw after them. The first withdrawal will correctly compute the vault's TVL, however it'll also withdraw all liquidity from ticks and won't re-deploy it, setting TVL to 0 for all other users.

Tools Used

Manual review

Recommended Mitigation Steps

In the GeVault.getTVL() function, consider counting current vault's token balances as part of its TVL. Since funds are not re-deployed when the vault is paused, they'll remain in the contract. But since they're removed from Uniswap pools and the lending pool, the current implementation doesn't count them as part of the TVL.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/4b785d455fff04629d8675f21ef1d1632749b252/contracts/GeVault.sol#L407

Vulnerability details

Impact

Users can lose a portion of their deposited funds if some of their funds haven't been deposited to the underlying Uniswap pools. There's always a chance of such event since Uniswap pools take balanced token amounts when liquidity is added but GeVault doesn't pre-compute balanced amounts. As a result, depositing and withdrawing can result in a partial loss of funds.

Proof of Concept

The GeVault.deposit() function is used by users to deposits funds into ticks and underlying Uniswap pools. The function takes funds from the caller and calls rebalance() to distribute the funds among the ticks. The GeVault.rebalance() function first removes liquidity from all ticks and then deposits the removed assets plus the user assets back in to the ticks:

function rebalance() public {
    require(poolMatchesOracle(), "GEV: Oracle Error");
    removeFromAllTicks();
    if (isEnabled) deployAssets();
}

The GeVault.deployAssets() function calls the GeVault.depositAndStash() function, which actually deposits tokens into a TokenisableRange contract by calling the TokenisableRange.deposit(). The function deposits tokens into a Uniswap V3 pool and returns unspent tokens to the caller:

(uint128 newLiquidity, uint256 added0, uint256 added1) = POS_MGR.increaseLiquidity(
    ...
);

...

_mint(msg.sender, lpAmt);
TOKEN0.token.safeTransfer( msg.sender, n0 - added0);
TOKEN1.token.safeTransfer( msg.sender, n1 - added1);

However, the GeVault.depositAndStash() function doesn't handle the returned unspent tokens. Since Uniswap V3 pools take balanced token amounts (respective to the current pool price) and since the funds deposited into ticks are not balanced (deployAssets() splits token amounts in halves), there's always a chance that the TokenisableRange.deposit() function won't consume all specified tokens and will return some of them to the GeVault contract. However, GeVault won't return the unused tokens to the depositor.

Moreover, the contract won't include them in the TVL calculation:

1. The GeVault.getTVL() function computes the total LP token balance of the contract (getTickBalance(k)) and the price of each LP token (t.latestAnswer()), to compute the total value of the vault.
2. The GeVault.getTickBalance() function won't count the unused tokens because it only returns the amount of LP tokens deposited into the lending pool. I.e. only the liquidity deposited to Uniswap pools is counted.
3. The TokenisableRange.latestAnswer() function computes the total value (TokenisableRange.sol#L355) of the liquidity deposited into the Uniswap pool (TokenisableRange.sol#L338). Thus, the unused tokens won't be counted here as well.
4. The GeVault.getTVL() function is used to compute the amount of tokens to return to the depositor during withdrawal.

Thus, the unused tokens will be locked in the contract until they're deposited into ticks. However, rebalancing and depositing of tokens can result in new unused tokens that won't be counted in the TVL.

Tools Used

Manual review

Recommended Mitigation Steps

In the GeVault.deposit() function, consider returning unspent tokens to the depositor. Extra testing is needed to guarantee that rebalancing doesn't result in unspent tokens, or, alternatively, such tokens could be counted in a storage variable and excluded from the balance of unspent tokens during depositing. Alternatively, consider counting GeVault's token balances in the getTVL() function. This won't require returning unspent tokens during depositing and will allow depositors to withdraw their entire funds.

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/4b785d455fff04629d8675f21ef1d1632749b252/contracts/RangeManager.sol#L100

Vulnerability details

Impact

The owner of the RangeManager contract will lose funds during initialization of a range. Since the actual amounts of tokens deposited to a Uniswap V3 pool are computed on-chain during depositing, there's always a chance that some amounts provided by the owner won't be deposited and will be left in the contract. The funds can later be withdrawn by anyone.

Proof of Concept

The RangeManager.initRange() function lets the owner initialize a previously deployed TokenisableRange contract. During the initialization, the owner deposits funds to the underlying Uniswap V3 pool of the TokenisableRange contract by calling TokenisableRange.init():

TokenisableRange(tr).init(amount0, amount1);

The TokenisableRange.init() function deposits the tokens to the Uniswap V3 pool and returns unused tokens to the caller:

(tokenId, liquidity, , ) = POS_MGR.mint( 
    ...
);

// Transfer remaining assets back to user
TOKEN0.token.safeTransfer( msg.sender,  TOKEN0.token.balanceOf(address(this)));
TOKEN1.token.safeTransfer(msg.sender, TOKEN1.token.balanceOf(address(this)));

However, the RangeManager.initRange() function doesn't sent those tokens back to the owner: it only transfers the LP tokens. As a result, the unused tokens will remain locked in the contract. Of course, the owner can call initRange() again to initialize another range and spent the leftover tokens. However, this call can also result in unspent tokens, thus there's always a chance that some tokens belonging to the owner will be left in the contract.

The leftover tokens can still be removed via the RangeManager.cleanup() function, which is called only in the RangeManager.removeAssetsFromStep(), which can be called by anyone. Consider this attack scenario:

1. The owner of RangeManager creates a new range and initializes it to add liquidity.
2. After the initialization, some tokens are left in the contract.
3. A MEV bot adds tiny amounts of liquidity to the range contract (via TokenisableRange.deposit()) and deposits the received LP tokens to the liquidity pool (forked from Aave), receiving respective aTokens.
4. The MEV bot calls RangeManager.removeFromStep(), which redeems the aTokens back to asset tokens and calls cleanup() to transfer all asset tokens owned by the contract (which includes the owner leftover tokens) to the MEV bot.
5. As a result, the MEV bot has stolen the leftover contracts that were not returned to the owner during the initialization of a new range.

Tools Used

Manual review

Recommended Mitigation Steps

In the RangeManager.initRange() function, in addition to returning LP tokens to the caller, consider also return the asset tokens (ASSET_0 and ASSET_1) to the caller.

Assessed type

Token-Transfer