search

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

3 stars 2 forks source link

Deposit will always revert when depositing ETH if one of the GeVault tokens is not WETH #493

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/GeVault.sol#L247-L284

Vulnerability details

Impact

When a user deposits funds, the deposit function requires an argument namely, the address of the token with which to fund the transaction.

If the GeVault has two tokens of for example: USDC/DAI. Line 251 expects the function argument to be the address of one of those. However according to lines 255 and 256, if there is a msg.value it means the user desires to deposit ETH into the vault, and the function argument needs to be the valid WETH address.

Thus if one of the Vault tokens is not WETH, the two checks work against each other and the call will always revert when depositing ETH.

The check is done within the deposit function especially lines 251 and 255,256. line 251:

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

lines 255,256:

    if (msg.value > 0){
      require(token == address(WETH), "GEV: Invalid Weth");

Proof of Concept

I created a miniGeVault contract in order to have the deposit functionality isolated, to make an easy PoC to run in foundry.

(Copy/paste into miniGeVault.sol)

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.19;

import "./openzeppelin-solidity/contracts/access/Ownable.sol";
import "./openzeppelin-solidity/contracts/token/ERC20/ERC20.sol";
import "./openzeppelin-solidity/contracts/token/ERC20/utils/SafeERC20.sol";
import "./openzeppelin-solidity/contracts/security/ReentrancyGuard.sol";
import "../interfaces/IAaveLendingPoolV2.sol";
import "../interfaces/IUniswapV3Pool.sol";
import "../interfaces/IWETH.sol";
import "./RangeManager.sol";
import "./RoeRouter.sol";
contract miniGeVault is ERC20, Ownable, ReentrancyGuard {
  using SafeERC20 for ERC20;

  event Deposit(address indexed sender, address indexed token, uint amount, uint liquidity);

  RangeManager rangeManager; 
  /// @notice Ticks properly ordered in ascending price order
  TokenisableRange[] public ticks;

  /// @notice Tracks the beginning of active ticks: the next 4 ticks are the active
  uint public tickIndex; 
  /// @notice Pair tokens
  ERC20 public token0;
  ERC20 public token1;
  bool public isEnabled = true;
  /// @notice Pool base fee 
  uint public baseFeeX4 = 20;
  /// @notice Max vault TVL with 8 decimals
  uint public tvlCap = 1e12;

  /// CONSTANTS 
  /// immutable keyword removed for coverage testing bug in brownie
  address public treasury;
  IWETH public WETH;
  bool public baseTokenIsToken0;
  IPriceOracle public oracle;

  //Constructor altered for ease of PoC
  constructor(
    address _token0, 
    address _token1)
    ERC20("GEFAIL", "GEFAIL")
  {
    token0 = ERC20(_token0);
    token1 = ERC20(_token1);
    WETH = IWETH(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);
  }

  /// @notice deposit tokens in the pool, convert to WETH if necessary
  /// @param token Token address
  /// @param amount Amount of token deposited
  function deposit(address token, uint amount) public payable nonReentrant returns (uint liquidity) 
  {
    require(isEnabled, "GEV: Pool Disabled");
    //NB!!! ########Commented out assuming WETH USDC AND DAI ARE VALID
    //require(poolMatchesOracle(), "GEV: Oracle Error");

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

    require(amount > 0 || msg.value > 0, "GEV: Deposit Zero");

    // Wrap if necessary and deposit here
    if (msg.value > 0){
      require(token == address(WETH), "GEV: Invalid Weth");
      // wraps ETH by sending to the wrapper that sends back WETH
      WETH.deposit{value: msg.value}();
      amount = msg.value;
    }
    else { 
      ERC20(token).safeTransferFrom(msg.sender, address(this), amount);
    }

    // Send deposit fee to treasury
    // NB!!!! ##### COMMENTED OUT AS DEPOSIT SHOULD FAIL
    //uint fee = amount * getAdjustedBaseFee(token == address(token0)) / 1e4;
    uint fee = 1e4;
    ERC20(token).safeTransfer(treasury, fee);
    uint valueX8 = oracle.getAssetPrice(token) * (amount - fee) / 10**ERC20(token).decimals();

    // NB!!!! ##### COMMENTED OUT AS DEPOSIT SHOULD FAIL
    //require(tvlCap > valueX8 + getTVL(), "GEV: Max Cap Reached");
    // NB!!!! ##### COMMENTED OUT AS DEPOSIT SHOULD FAIL
    //uint vaultValueX8 = getTVL();
    // REPLACE WITH THIS LINE JUST TO COMPILE
    uint vaultValueX8 = 1;
    uint tSupply = totalSupply();
    // initial liquidity at 1e18 token ~ $1
    if (tSupply == 0 || vaultValueX8 == 0)
      liquidity = valueX8 * 1e10;
    else {
      liquidity = tSupply * valueX8 / vaultValueX8;
    }

    // NB!!!! ##### COMMENTED OUT AS DEPOSIT SHOULD FAIL
    //rebalance();
    require(liquidity > 0, "GEV: No Liquidity Added");
    _mint(msg.sender, liquidity);    
    emit Deposit(msg.sender, token, amount, liquidity);
  }  
}

(Copy/paste into GeVault.t.sol)

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console2} from "forge-std/Test.sol";
import "../contracts/miniGeVault.sol";

contract GeVaultTest is Test {
    address public USDC = 0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48;
    address public WETH = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;
    address public FAKEDAI = address(123);
    miniGeVault miniVault;

    function setUp() public {
        // SETUP NEW VAULT WITH TOKENS AS USDC AND DAI
        miniVault = new miniGeVault(FAKEDAI,USDC);
    }

    function testDepositETH() public {
        // EXPECT REVERT BECAUSE THE TOKEN WILL BE INVALID
        vm.expectRevert();
        miniVault.deposit{value: 1 ether}(WETH,0);
    }

    function testDepositSayingitsUSDC() public {
        // EXPECT REVERT BECAUSE THE TOKEN WILL BE INVALID
        vm.expectRevert();
        miniVault.deposit{value: 1 ether}(USDC,0);
    }
}

To run the test: forge test -vv --match-contract GeVaultTest

The output will be :

Running 2 tests for test/GeVault.t.sol:GeVaultTest
[PASS] testDepositETH() (gas: 26818)
[PASS] testDepositSayingitsUSDC() (gas: 28981)
Test result: ok. 2 passed; 0 failed; 0 skipped; finished in 1.20ms
Ran 1 test suites: 2 tests passed, 0 failed, 0 skipped (2 total tests)

Tools Used

Assessed type

Payable

c4-pre-sort commented 1 year ago

141345 marked the issue as primary issue

c4-sponsor commented 1 year ago

Keref marked the issue as sponsor disputed

Keref commented 1 year ago

POC shows that depositing ETH in a non ETH pool reverts, which is expected. When depositing ETH in a ETH pool, bc underlying ETH is converted to WETH by uniswap, the token address passed should be WETH.

c4-judge commented 1 year ago

gzeon-c4 marked the issue as unsatisfactory: Invalid