Good Entry - Krace's results

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/helper/V3Proxy.sol#L160

Vulnerability details

Impact

swapTokensForExactETH is capable of receiving tokens and then converting them into the specified amount of ETH to return to the user. However, after the exchange, the remaining tokens are not returned to the user, resulting in a loss of funds for the user. It simply clears the approval but does not return the remaining ogInAsset tokens after the exchange to the user.

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/helper/V3Proxy.sol#L160

    function swapTokensForExactETH(uint amountOut, uint amountInMax, address[] calldata path, address to, uint deadline) payable external returns (uint[] memory amounts) {
        require(path.length == 2, "Direct swap only");
        require(path[1] == ROUTER.WETH9(), "Invalid path");
        ERC20 ogInAsset = ERC20(path[0]);
        ogInAsset.safeTransferFrom(msg.sender, address(this), amountInMax);
        ogInAsset.safeApprove(address(ROUTER), amountInMax);
        amounts = new uint[](2);
        amounts[0] = ROUTER.exactOutputSingle(ISwapRouter.ExactOutputSingleParams(path[0], path[1], feeTier, address(this), deadline, amountOut, amountInMax, 0));         
        amounts[1] = amountOut;  
        // => we should return ogInAsset to user
        ogInAsset.safeApprove(address(ROUTER), 0); 
        IWETH9 weth = IWETH9(ROUTER.WETH9());
        acceptPayable = true;
        weth.withdraw(amountOut);
        acceptPayable = false;
        payable(msg.sender).call{value: amountOut}("");
        emit Swap(msg.sender, path[0], path[1], amounts[0], amounts[1]); 
    }

Proof of Concept

Consider the following scenario:

1. Alice has 10000 USDT and she wants to get 1 ETH
2. The price of 1 ETH is 9000 USDT
3. Alice calls swapTokensForExactETH(1, 10000, [address(USDT), address(WETH)], address(0), 0)
4. Alice receive 1ETH. However, her remaining 1000 USDT is left in the V3Proxy contract and cannot be retrieved.

Tools Used

VSCode

Recommended Mitigation Steps

After exchanging WETH, return the remaining ogInAsset tokens to the msg.sender.

diff --git a/contracts/helper/V3Proxy.sol b/contracts/helper/V3Proxy.sol
index 45ab57e..190b5cb 100644
--- a/contracts/helper/V3Proxy.sol
+++ b/contracts/helper/V3Proxy.sol
@@ -166,6 +166,7 @@ contract V3Proxy is ReentrancyGuard, Ownable {
         amounts = new uint[](2);
         amounts[0] = ROUTER.exactOutputSingle(ISwapRouter.ExactOutputSingleParams(path[0], path[1], feeTier, address(this), deadline, amountOut, amountInMax, 0));
         amounts[1] = amountOut;
+        ogInAsset.safeTransfer(msg.sender, ogInAsset.balanceOf(address(this)));
         ogInAsset.safeApprove(address(ROUTER), 0);
         IWETH9 weth = IWETH9(ROUTER.WETH9());
         acceptPayable = true;

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

GeVault::modifyTick can modify the elements of ticks without the need to maintain the order of tick. If the owner wrongly modified the tick and destroy the order of ticks, the normal function of contract TokenisableRange will be compromised.

Proof of Concept

As shown in the code, the owner could modify the elements of ticks without any restrictions.

  /// @notice Modify ticker
  /// @param tr New tick address
  /// @param index Tick to modify
  function modifyTick(address tr, uint index) public onlyOwner {
    (ERC20 t0,) = TokenisableRange(tr).TOKEN0();
    (ERC20 t1,) = TokenisableRange(tr).TOKEN1();
    require(t0 == token0 && t1 == token1, "GEV: Invalid TR");
    ticks[index] = TokenisableRange(tr);
    emit ModifyTick(tr, index);
  }

Tools Used

VSCode

Recommended Mitigation Steps

Add some checks to ensure that the modified Tick can meet the sequencing requirements.

diff --git a/contracts/GeVault.sol b/contracts/GeVault.sol
index 4c9dae3..f58a60f 100644
--- a/contracts/GeVault.sol
+++ b/contracts/GeVault.sol
@@ -165,10 +165,28 @@ contract GeVault is ERC20, Ownable, ReentrancyGuard {
   /// @param tr New tick address
   /// @param index Tick to modify
   function modifyTick(address tr, uint index) public onlyOwner {
-    (ERC20 t0,) = TokenisableRange(tr).TOKEN0();
-    (ERC20 t1,) = TokenisableRange(tr).TOKEN1();
+    TokenisableRange t = TokenisableRange(tr);
+    (ERC20 t0,) = t.TOKEN0();
+    (ERC20 t1,) = t.TOKEN1();
     require(t0 == token0 && t1 == token1, "GEV: Invalid TR");
-    ticks[index] = TokenisableRange(tr);
+    // left
+    if (index - 1 >= 0){
+      if (baseTokenIsToken0)
+        require( t.lowerTick() > ticks[index - 1].upperTick(), "GEV: Modify Tick Overlap");
+      else
+        require( t.upperTick() < ticks[index - 1].lowerTick(), "GEV: Modify Tick Overlap");
+    }
+    // right
+    if (index < ticks.length - 1){
+      if (baseTokenIsToken0)
+        require( t.upperTick() < ticks[index + 1].lowerTick(), "GEV: Modify Tick Overlap");
+      else
+        require( t.lowerTick() > ticks[index + 1].upperTick(), "GEV: Modify Tick Overlap");
+    }
+    ticks[index] = t;
+    //(ERC20 t0,) = TokenisableRange(tr).TOKEN0();
+    //(ERC20 t1,) = TokenisableRange(tr).TOKEN1();
+    //ticks[index] = TokenisableRange(tr);
     emit ModifyTick(tr, index);
   }

Assessed type

DoS