code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The withdraw function of the GeVault contract does not verify whether the token parameter belongs to token0 or token1, which poses a potential risk of causing user losses. Assuming that when the withdraw function is called, the incoming address token is some garbage, fraudulent token, or malicious token, the user will not be able to obtain anything valuable, and their GEV token share will be burn normally.

I know this is a loss caused by the user's own foolish behavior, but as long as we determine whether the incoming address token belongs to token0 or token1 in the withdraw function, we can avoid this loss. Verifying token parameters is beneficial without any harm. Therefore, please consider it as an [M].

Proof of Concept

In the GeVault contract, there is a withdraw function code as follows. It can be seen that the code does not determine whether the incoming address token belongs to token0 or token1

function withdraw(uint liquidity, address token) public nonReentrant returns (uint amount) {
    require(poolMatchesOracle(), "GEV: Oracle Error");
    if (liquidity == 0) liquidity = balanceOf(msg.sender);
    require(liquidity <= balanceOf(msg.sender), "GEV: Insufficient Balance");
    require(liquidity > 0, "GEV: Withdraw Zero");

    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;

    _burn(msg.sender, liquidity);
    removeFromAllTicks();
    ERC20(token).safeTransfer(treasury, fee);
    uint bal = amount - fee;

    if (token == address(WETH)){
      WETH.withdraw(bal);
      payable(msg.sender).transfer(bal);
    }
    else {
      ERC20(token).safeTransfer(msg.sender, bal);
    }

    // if pool enabled, deploy assets in ticks, otherwise just let assets sit here until totally withdrawn
    if (isEnabled) deployAssets();
    emit Withdraw(msg.sender, token, amount, liquidity);
  }

Tools Used

Visual Studio Code Foundry

Recommended Mitigation Steps

Add a verification statement in the withdraw function:

require(token == address(token0) || token == address(token1), "GEV: Invalid Token");

Assessed type

Invalid Validation

c4-pre-sort commented 1 year ago

141345 marked the issue as duplicate of #339

c4-judge commented 1 year ago

gzeon-c4 changed the severity to QA (Quality Assurance)