Good Entry - Sathish9098's results

Lines of code

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

Vulnerability details

Impact

The payable(msg.sender).call{value: amountOut}("") function does not check the status of the call, so it is possible to lose funds if the call fails.

Proof of Concept

FILE: 2023-08-goodentry/contracts/helper/V3Proxy.sol

156: msg.sender.call{value: msg.value - amounts[0]}("");

174: payable(msg.sender).call{value: amountOut}("");

192: payable(msg.sender).call{value: amounts[1]}("");

POC

The call function is a low-level function that allows you to send a message to another contract. The call function does not check the status of the call, so it is possible for the call to fail for a variety of reasons

• The recipient contract may be invalid
• The recipient contract may not have enough gas to execute the call
• The recipient contract may throw an exception
• The recipient not implemented the fallback functions

Tools Used

Manual Audit

Recommended Mitigation Steps

Implement return value check to avoid funds lose

+ (bool sent,) = payable(msg.sender).call{value: amountOut}("");

+ require(sent, " Call failed ")

Assessed type

Access Control

LOW FINDINGS

[L-1] Array is push()ed but not pop()ed

Impact

Array entries are added but are never removed. Consider whether this should be the case, or whether there should be a maximum, or whether old entries should be removed. Cases where there are specific potential problems will be flagged separately under a different issue.

POC

FILE: 2023-08-goodentry/contracts/GeVault.sol

121: if (ticks.length == 0) ticks.push(t);

130: ticks.push(TokenisableRange(tr));

152: ticks.push(ticks[ticks.length-1]);

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

FILE: Breadcrumbs2023-08-goodentry/contracts/RangeManager.sol

78: stepList.push( Step(startX10, endX10) );

80: tokenisedRanges.push( TokenisableRange(address(trbp)) );

82: tokenisedTicker.push( TokenisableRange(address(trbp)) );

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/RangeManager.sol#L78

FILE: Breadcrumbs2023-08-goodentry/contracts/RoeRouter.sol

76: pools.push(RoePool(lendingPoolAddressProvider, token0, token1, ammRouter, false));

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/RoeRouter.sol#L76

Recommended Mitigation

Use pop() method to remove elements

[L-3] Missing contract-existence checks before low-level calls

Impact

Low-level calls return success if there is no code present at the specified address. In addition to the zero-address checks, add a check to verify that <address>.code.length > 0

POC

FILE: Breadcrumbs2023-08-goodentry/contracts/helper/V3Proxy.sol

174: payable(msg.sender).call{value: amountOut}("");

192: payable(msg.sender).call{value: amounts[1]}("");

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

Recommended Mitigation

Add <address>.code.length > 0 this check

[L-4] External call recipient may consume all transaction gas

Impact

There is no limit specified on the amount of gas used, so the recipient can use up all of the transaction's gas, causing it to revert.

FILE: Breadcrumbs2023-08-goodentry/contracts/helper/V3Proxy.sol

174: payable(msg.sender).call{value: amountOut}("");

192: payable(msg.sender).call{value: amounts[1]}("");

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

Recommended Mitigation

Use addr.call{gas: <amount>}("") or this library instead.

[L-5] Allowed fees/rates should be capped by smart contracts

Impact

Fees/rates should be required to be below 100%, preferably at a much lower limit, to ensure users don't have to monitor the blockchain for changes prior to using the protocol

 /// @notice Max vault TVL with 8 decimals

POC

FILE: Breadcrumbs2023-08-goodentry/contracts/GeVault.sol

190: function setTvlCap(uint newTvlCap) public onlyOwner {
191:    tvlCap = newTvlCap;
192:    emit SetTvlCap(newTvlCap);
193:   }

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

Recommended Mitigation

Add sanity checks (Min,Max,>0) when assigning uint values

[L-6] Consider implementing two-step procedure for updating protocol addresses

Impact

A copy-paste error or a typo may end up bricking protocol functionality, or sending tokens to an address with no known private key. Consider implementing a two-step procedure for updating protocol addresses, where the recipient is set as pending, and must 'accept' the assignment by making an affirmative call. A straight forward way of doing this would be to have the target contracts implement EIP-165, and to have the 'set' functions ensure that the recipient is of the right interface type.

POC

FILE: 2023-08-goodentry/contracts/RoeRouter.sol

 function setTreasury(address newTreasury) public onlyOwner {
    require(newTreasury != address(0x0), "Invalid address");
    treasury = newTreasury;
    emit UpdateTreasury(newTreasury);
  }

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/RoeRouter.sol#L83-L87

FILE: Breadcrumbs2023-08-goodentry/contracts/GeVault.sol

function setTreasury(address newTreasury) public onlyOwner { 
    treasury = newTreasury; 
    emit SetTreasury(newTreasury);
  }

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

Recommended Mitigation

Add two-step procedure when updating protocol addresses

[L-7] safeApprove() is deprecated

Impact

Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance(). If only setting the initial allowance to the value that means infinite, safeIncreaseAllowance() can be used instead. The function may currently work, but if a bug is found in this version of OpenZeppelin, and the version that you're forced to upgrade to no longer has this function, you'll encounter unnecessary delays in porting and testing replacement contracts.

POC

FILE: 2023-08-goodentry/contracts/helper/V3Proxy.sol

116: ogInAsset.safeApprove(address(ROUTER), amountIn);

120: ogInAsset.safeApprove(address(ROUTER), 0);

128: ogInAsset.safeApprove(address(ROUTER), amountInMax);

133: ogInAsset.safeApprove(address(ROUTER), 0);

165: ogInAsset.safeApprove(address(ROUTER), amountInMax);

169: ogInAsset.safeApprove(address(ROUTER), 0);

183: ogInAsset.safeApprove(address(ROUTER), amountIn);

187: ogInAsset.safeApprove(address(ROUTER), 0); 

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

Recommended Mitigation

Use safeIncreaseAllowance() and safeDecreaseAllowance()

[L-8] Calls to withdraw() will revert when totalSupply() returns zero

Impact

totalSupply() being zero will result in a division by zero, causing the transaction to fail. The function should instead special-case this scenario, and avoid reverting.

POC

FILE: 2023-08-goodentry/contracts/GeVault.sol

221: uint valueX8 = vaultValueX8 * liquidity / totalSupply();

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

FILE: Breadcrumbs2023-08-goodentry/contracts/TokenisableRange.sol

371: (token0Amount, token1Amount) = LiquidityAmounts.getAmountsForLiquidity( sqrtPriceX96, TickMath.getSqrtRatioAtTick(lowerTick), TickMath.getSqrtRatioAtTick(upperTick),  uint128 ( uint(liquidity) * amount / totalSupply() ) );

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/TokenisableRange.sol#L371

Recommended Mitigation

totalSupply() must be non zero

[L-2] Some tokens may revert when zero value transfers are made

Impact

In spite of the fact that EIP-20 states that zero-valued transfers must be accepted, some tokens, such as LEND will revert if this is attempted, which may cause transactions that involve other tokens (such as batch operations) to fully revert. Consider skipping the transfer if the amount is zero, which will also save gas.

POC

FILE: 2023-08-goodentry/contracts/GeVault.sol

227: ERC20(token).safeTransfer(treasury, fee);

235: ERC20(token).safeTransfer(msg.sender, bal);

267: ERC20(token).safeTransfer(treasury, fee);

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

Recommended Mitigation

Check zero amount before call transfer function

[L-9] Hardcoded treasuryFee may cause problem in future

Impact

The fee may be difficult to change, which can make it difficult to adapt to changes in the market

POC

FILE: 2023-08-goodentry/contracts/TokenisableRange.sol

61: uint constant public treasuryFee = 20;

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/TokenisableRange.sol#L61

Recommended Mitigation

Make sure treasuryFee easy to change when necessary

[L-10] Remove deprecated variables to avoid confusion

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/TokenisableRange.sol#L53-L55

[L-11] Add view when evert states are not changed

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/PositionManager/OptionsPositionManager.sol#L533

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/PositionManager/OptionsPositionManager.sol#L544

[L-12] Pool deprecated status is not checked when accessing pools

Impact

The deprecatePool function does not check the deprecated status of the pool before accessing it. This means that it is possible to call the getPool function with a deprecated pool id, and the function will return the pool's data, even though the pool is deprecated

POC

FILE: 2023-08-goodentry/contracts/PositionManager/PositionManager.sol

function getPoolAddresses(uint poolId) 
    internal view 
    returns( ILendingPool lp, IPriceOracle oracle, IUniswapV2Router01 router, address token0, address token1) 
  {
    (address lpap, address _token0, address _token1, address r, ) = ROEROUTER.pools(poolId);
    token0 = _token0;
    token1 = _token1;
    lp = ILendingPool(ILendingPoolAddressesProvider(lpap).getLendingPool());
    oracle = IPriceOracle(ILendingPoolAddressesProvider(lpap).getPriceOracle());
    router = IUniswapV2Router01(r);
  }

FILE: Breadcrumbs2023-08-goodentry/contracts/RoeRouter.sol

function deprecatePool(uint poolId) public onlyOwner {
    pools[poolId].isDeprecated = true;
    emit DeprecatePool(poolId);
  }

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/PositionManager/PositionManager.sol#L63C3-L73C4

Recommended Mitigation

Add check to ensure the pool is deprecated or not

[L-13] Shut off the renounceOwnership() function to avoid unexpected behavior

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

[L-14] Don’t use payable(address).call()

Impact

payable(address).call() is a low-level method that can be used to send ether to a contract, but it has some limitations and risks as you've pointed out. One of the primary risks of using payable(address).call() is that it doesn't guarantee that the contract's payable function will be called successfully. This can lead to funds being lost or stuck in the contract

The contract does not have a payable callback The contract’s payable callback spends more than 2300 gas (which is only enough to emit something) The contract is called through a proxy which itself uses up the 2300 gas Use OpenZeppelin’s Address.sendValue() instead

FILE: Breadcrumbs2023-08-goodentry/contracts/helper/V3Proxy.sol

174: payable(msg.sender).call{value: amountOut}("");

192: payable(msg.sender).call{value: amounts[1]}("");

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

Recommended Mitigation

Use OpenZeppelin’s Address.sendValue()

[L-15] Lack of control for liquidate()

Impact

Anyone can call and can initiate the liquidations . This may creates unintended consequences.

POC

FILE: Breadcrumbs2023-08-goodentry/contracts/PositionManager/OptionsPositionManager.sol

function liquidate (
    uint poolId, 
    address user,
    address[] memory options, 
    uint[] memory amounts,
    address collateralAsset
  )
    external

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/PositionManager/OptionsPositionManager.sol#L189-L197

Recommended Mitigation

Add control like onlyOwner()

[L-16] oracle.getAssetPrice(collateralAsset) this will revert some cases if collateralAsset is not exists

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/PositionManager/OptionsPositionManager.sol#L367

[L-17] There no deadline and no slippage protections when swapping tokens

Impact

The swapTokens function does not have a deadline or slippage protection. This means that there is no guarantee that the swap will be executed, and the user may lose money if the price of the tokens changes significantly during the swap.

A deadline is a time limit that is set for a transaction. If the transaction is not executed before the deadline, it is reverted. This can be useful for preventing users from losing money if the price of the tokens changes significantly during the swap.

Slippage protection is a feature that limits the amount of slippage that can occur during a swap. Slippage is the difference between the expected price of a swap and the actual price of the swap. If the price of the tokens changes significantly during the swap, the user may lose money due to slippage

POC

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/PositionManager/OptionsPositionManager.sol#L439-L456

Recommended Mitigation

Add minOutAmount and deadline in swapTokens() functions

GAS OPTIMIZATIONS

[G-1] Using calldata instead of memory for read-only arguments in external functions saves gas

Saves 1974 GAS

When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Note that even if an interface defines a function as having memory arguments, it’s still valid for implementation contracs to use calldata arguments instead.

If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it’s still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one

Note that I’ve also flagged instances where the function is public but can be marked as external since it’s not called by the contract, and cases where a constructor is involved

options, amounts, sourceSwap can be changed to calldata instead of memory. Because read-only arguments : Saves 846 GAS

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/PositionManager/OptionsPositionManager.sol#L159-L165

FILE: 2023-08-goodentry/contracts/PositionManager/OptionsPositionManager.sol

159: function buyOptions(
160:    uint poolId, 
+ 161:    address[] calldata options, 
- 161:    address[] memory options, 
+ 162:    uint[] calldata amounts, 
- 162:    uint[] memory amounts, 
+ 163:    address[] memory sourceSwap
- 163:    address[] memory sourceSwap
164:  )
165:    external
166:  {

options, amounts can be changed to calldata instead of memory. Because read-only arguments: Saves 564 GAS

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/PositionManager/OptionsPositionManager.sol#L189-L196

FILE: 2023-08-goodentry/contracts/PositionManager/OptionsPositionManager.sol

189: function liquidate (
190:    uint poolId, 
191:    address user,
+ 192:    address[] memory options, 
- 192:    address[] memory options, 
+ 193:    uint[] memory amounts,
- 193:    uint[] memory amounts,
194:    address collateralAsset
195:  )
196:    external
197:  {

startName, endName can be changed to calldata instead of memory. Because read-only arguments : Saves 564 GAS

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/RangeManager.sol#L75

FILE: 2023-08-goodentry/contracts/RangeManager.sol

- 75: function generateRange(uint128 startX10, uint128 endX10, string memory startName, string memory endName, address beacon) external onlyOwner {

+ 75: function generateRange(uint128 startX10, uint128 endX10, string calldata startName, string calldata endName, address beacon) external onlyOwner {

[G-2] State variables can be packed into fewer storage slots

Saves 4000 GAS, 2 SLOT

The EVM works with 32 byte words. Variables less than 32 bytes can be declared next to eachother in storage and this will pack the values together into a single 32 byte storage slot (if the values combined are <= 32 bytes). If the variables packed together are retrieved together in functions we will effectively save ~2000 gas with every subsequent SLOAD for that storage slot. This is due to us incurring a Gwarmaccess (100 gas) versus a Gcoldsload (2100 gas).

baseFeeX4 can be declared uint96 instead of uint : Saves 2000 GAS, 1 SLOT

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

baseFeeX4 value is not exceed 1e4 because require(newBaseFeeX4 < 1e4, "GEV: Invalid Base Fee"); this check

FILE: Breadcrumbs2023-08-goodentry/contracts/GeVault.sol

48:  ERC20 public token0;
+ 51:  /// @notice Pool base fee 
+ 52:  uint96 public baseFeeX4 = 20;
49:  ERC20 public token1;
50:  bool public isEnabled = true;
- 51:  /// @notice Pool base fee 
- 52:  uint public baseFeeX4 = 20;
53:  /// @notice Max vault TVL with 8 decimals
54:  uint public tvlCap = 1e12;

lowerTick,upperTick,feeTier,liquidity can be packed together : Saves 2000 GAS, 1 SLOT

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/TokenisableRange.sol#L52

FILE: Breadcrumbs2023-08-goodentry/contracts/TokenisableRange.sol

  int24 public lowerTick;
  int24 public upperTick;
  uint24 public feeTier;
+  uint128 public liquidity;
  
  uint256 public tokenId;
  uint256 public fee0;
  uint256 public fee1;
  
  struct ASSET {
    ERC20 token;
    uint8 decimals;
  }
  
  ASSET public TOKEN0;
  ASSET public TOKEN1;
  IAaveOracle public ORACLE;
  
  string _name;
  string _symbol;
  
  enum ProxyState { INIT_PROXY, INIT_LP, READY }
  ProxyState public status;
  address private creator;
  
-  uint128 public liquidity;
  // @notice deprecated, keep to avoid beacon storage slot overwriting errors
  address public TREASURY_DEPRECATED = 0x22Cc3f665ba4C898226353B672c5123c58751692;
  uint public treasuryFee_deprecated = 20;

[G-3] Structs can be packed into fewer storage slots

Saves 4000 GAS, 2 SLOTs

Each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct.

deadline can be downcasted to uint96 instead of uint256 : saves 4000 GAS, 2 SLOTs.

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

The current timestamp is 2023-08-07 04:44:39 PST. The maximum timestamp that can be stored in a uint96 is 2**96 - 1, which is about 1.8446744073709552e+18 seconds. This is more than 276 years, so it will take at least 276 years for the block timestamp to overflow when stored in a uint96

FILE: 2023-08-goodentry/contracts/helper/V3Proxy.sol

10: struct ExactInputSingleParams {
11:        address tokenIn;
12:        address tokenOut;
13:        uint24 fee;
14:        address recipient;
+ 15:        uint96 deadline;
- 15:        uint256 deadline;
16:        uint256 amountIn;
17:        uint256 amountOutMinimum;
18:        uint160 sqrtPriceLimitX96;
19:    }


22: struct ExactOutputSingleParams {
23:        address tokenIn;
24:        address tokenOut;
25:        uint24 fee;
26:        address recipient;
+ 27:        uint96 deadline;
- 27:        uint256 deadline;
28:        uint256 amountOut;
29:        uint256 amountInMaximum;
30:        uint160 sqrtPriceLimitX96;
31:    }

[G-4] Using bools for storage incurs overhead

Saves 60000 GAS, 3 Instances

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past.

FILE: Breadcrumbs2023-08-goodentry/contracts/GeVault.sol

50: bool public isEnabled = true;

64: bool public baseTokenIsToken0;

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

FILE: 2023-08-goodentry/contracts/helper/V3Proxy.sol

65: bool acceptPayable;

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

[G-5] Functions guaranteed to revert when called by normal users can be marked payable

Saves 273 GAS, 13 Instances

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

101:   function setEnabled(bool _isEnabled) public onlyOwner  

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

108:   function setTreasury(address newTreasury) public onlyOwner  

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

116:   function pushTick(address tr) public onlyOwner  

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

137:   function shiftTick(address tr) public onlyOwner  

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

167:   function modifyTick(address tr, uint index) public onlyOwner  

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

183:   function setBaseFee(uint newBaseFeeX4) public onlyOwner  

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

191:   function setTvlCap(uint newTvlCap) public onlyOwner  

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

94:     function emergencyWithdraw(ERC20 token) onlyOwner external  

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/helper/V3Proxy.sol#L94-L94

75:   function generateRange(uint128 startX10, uint128 endX10, string memory startName, string memory endName, address beacon) external onlyOwner  

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/RangeManager.sol#L75-L75

95:   function initRange(address tr, uint amount0, uint amount1) external onlyOwner 

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/RangeManager.sol#L95-L95

48:   function deprecatePool(uint poolId) public onlyOwner 

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/RoeRouter.sol#L48-L48

59:   function addPool(
60:     address lendingPoolAddressProvider, 
61:     address token0, 
62:     address token1, 
63:     address ammRouter
64:   ) 
65:     public onlyOwner 
66:     returns (uint poolId)
67:   

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/RoeRouter.sol#L65-L65

24:     function setHardcodedPrice(int256 _hardcodedPrice) external onlyOwner 

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/helper/FixedOracle.sol#L24-L24

[G-6] Multiple accesses of a mapping/array should use a local variable cache

Saves 1100 GAS, 11 SLODs

The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping’s value in a local storage or calldata variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key’s keccak256 hash (Gkeccak256 - 30 gas) and that calculation’s associated stack operations. Caching an array’s struct avoids recalculating the array offsets into memory/calldata

ticks[ticks.length-1], ticks[0] should be cached with stack variable : Saves 200 GAS, 2 SLOD

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

FILE: Breadcrumbs2023-08-goodentry/contracts/GeVault.sol

122: else {
123:      // Check that tick is properly ordered
+   TokenisableRange ticks_ = ticks[ticks.length-1];
124:      if (baseTokenIsToken0) 
- 125:        require( t.lowerTick() > ticks[ticks.length-1].upperTick(), "GEV: Push Tick Overlap");
+ 125:        require( t.lowerTick() > ticks_.upperTick(), "GEV: Push Tick Overlap");
126:      else 
- 127:        require( t.upperTick() < ticks[ticks.length-1].lowerTick(), "GEV: Push Tick Overlap");
+ 127:        require( t.upperTick() < ticks_ .lowerTick(), "GEV: Push Tick Overlap");
128:      
129:      ticks.push(TokenisableRange(tr));


145: else {
146:      // Check that tick is properly ordered
+   TokenisableRange ticks_ = ticks[0];
147:      if (!baseTokenIsToken0) 
- 148:        require( t.lowerTick() > ticks[0].upperTick(), "GEV: Shift Tick Overlap");
+ 148:        require( t.lowerTick() > ticks_.upperTick(), "GEV: Shift Tick Overlap");
149:      else 
- 150:        require( t.upperTick() < ticks[0].lowerTick(), "GEV: Shift Tick Overlap");
+ 150:        require( t.upperTick() < ticks_.lowerTick(), "GEV: Shift Tick Overlap");

tokenisedTicker[step],tokenisedRanges[ tokenisedRanges.length - 1 ],tokenisedTicker[step] should be cached : Saves 800 GAS, 8 SLOD

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/RangeManager.sol#L111-L133

FILE: Breadcrumbs2023-08-goodentry/contracts/RangeManager.sol

+   TokenisableRange  step_ = tokenisedRanges[step] ; 
- 111: trAmt = ERC20(LENDING_POOL.getReserveData(address(tokenisedRanges[step])).aTokenAddress).balanceOf(msg.sender);  
+ 111: trAmt = ERC20(LENDING_POOL.getReserveData(address(step_)).aTokenAddress).balanceOf(msg.sender);  
112:    if (trAmt > 0) {       
113:        LENDING_POOL.PMTransfer(
- 114:          LENDING_POOL.getReserveData(address(tokenisedRanges[step])).aTokenAddress,
+ 114:          LENDING_POOL.getReserveData(address(step_)).aTokenAddress,  
115:          msg.sender, 
116:          trAmt
117:        );
- 118:        trAmt = LENDING_POOL.withdraw(address(tokenisedRanges[step]), type(uint256).max, address(this));
+ 118:        trAmt = LENDING_POOL.withdraw(address(step_), type(uint256).max, address(this));
- 119:        tokenisedRanges[step].withdraw(trAmt, 0, 0);
+ 119:        step_.withdraw(trAmt, 0, 0);
- 120:        emit Withdraw(msg.sender, address(tokenisedRanges[step]), trAmt);
+ 120:        emit Withdraw(msg.sender, address(step_), trAmt);
121: }        
122:
+     TokenisableRange  stepTicker_ = tokenisedTicker[step] ; 
- 123:    trAmt = ERC20(LENDING_POOL.getReserveData(address(tokenisedTicker[step])).aTokenAddress).balanceOf(msg.sender);
+ 123:    trAmt = ERC20(LENDING_POOL.getReserveData(address(stepTicker_)).aTokenAddress).balanceOf(msg.sender);
124:    if (trAmt > 0) {    
125:        LENDING_POOL.PMTransfer(
- 126:          LENDING_POOL.getReserveData(address(tokenisedTicker[step])).aTokenAddress, 
+ 126:          LENDING_POOL.getReserveData(address(stepTicker_)).aTokenAddress, 
127:          msg.sender, 
128:          trAmt
129:        );
- 130:        uint256 ttAmt = LENDING_POOL.withdraw(address(tokenisedTicker[step]), type(uint256).max, address(this));
+ 130:        uint256 ttAmt = LENDING_POOL.withdraw(address(stepTicker_), type(uint256).max, address(this));
- 131:        tokenisedTicker[step].withdraw(ttAmt, 0, 0);
+ 131:        stepTicker_.withdraw(ttAmt, 0, 0);
- 132:        emit Withdraw(msg.sender, address(tokenisedTicker[step]), trAmt);
+ 132:        emit Withdraw(msg.sender, address(stepTicker_), trAmt);
133:    }           

[G-7] State variable should be cached inside the loop

Saves 100 GAS, 1 SLOD for each iterations

State variable should be cached inside the loop if it is accessed multiple times in the loop. This is because accessing a state variable from memory is a relatively expensive operation, so caching the variable in a local variable will improve performance

stepList[i] should be cached : Saves 100 GAS, 1 SLOT for every iterations

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/RangeManager.sol#L62-L65

FILE: 2023-08-goodentry/contracts/RangeManager.sol

62: for (uint i = 0; i < len; i++) {
+   Step stepList_ = stepList[i];
- 63:      if (start >= stepList[i].end || end <= stepList[i].start) {
+ 63:      if (start >= stepList_.end || end <= stepList_.start) {
64:        continue;
65:      }

[G-8] IF’s/require() statements that check input arguments should be at the top of the function

Saves 400 GAS

FAIL CHEEPLY INSTEAD OF COSTLY

Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas) in a function that may ultimately revert in the unhappy case.

Function parameters should be checked top of the function : Saves 400 GAS

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

FILE: 2023-08-goodentry/contracts/GeVault.sol

+ 252:    require(amount > 0 || msg.value > 0, "GEV: Deposit Zero");
249:    require(isEnabled, "GEV: Pool Disabled");
250:    require(poolMatchesOracle(), "GEV: Oracle Error");
251:    require(token == address(token0) || token == address(token1), "GEV: Invalid Token");
- 252:    require(amount > 0 || msg.value > 0, "GEV: Deposit Zero");

[G-9] Consider using the view or pure keywords for functions that don't modify state to save gas

The view and pure keywords can be used to indicate that a function does not modify state. This can save gas, because the compiler will not need to check the gas costs of the function's internal operations

FILE: Breadcrumbs2023-08-goodentry/contracts/PositionManager/OptionsPositionManager.sol

533: function sanityCheckUnderlying(address tr, address token0, address token1) internal {

544: function addDust(address debtAsset, address token0, address token1) internal returns (uint amount){

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/PositionManager/OptionsPositionManager.sol#L533

Analysis - Good Entry

Approach taken in evaluating the codebase

Steps:

• Read all documents with very fast forward way

• Then read the Readme.md file understood following things

  • The test coverage is only 85%
  • Protocol uses Aave LP and Uniswap v3 positions implementations
  • Protocol uses EVM-compatible side-chain
  • Protocol uses Chainlink oracle
  • AMM, ERC-20 Token
  • list specific ERC20 that your protocol is anticipated to interact with USDC.e (bridged USDC), wBTC, ARB, GMX
  • Smart contracts deployed Arbitrum
  • EIP's used TokenizableRange.sol and GeVault.sol are ERC20 compliant

• GoodEntry is a perpetual options trading platform, or protected perps: user can trade perps with limited downside. It is built on top of Uniswap v3 and relies on single tick liquidity

  • Tokenized Uniswap v3 positions (TR), with a manager, oracle, and price manipulation resistance
  • ezVaults, holding TRs, and rebalancing the underlying liquidity as needed
  • A lending pool, forked from Aave v2 (out of audit scope)
  • A router that whitelists allowed addresses (such as allowed swap pools)

• Then clone the repo and setup test environment to make sure all written tests are working correct

• Then Jump into the codebase analysis. In first iteration just identified the important GAS and QA related findings.

GAS

1. Using calldata instead of memory for read-only arguments in external functions saves gas
2. State variables can be packed into fewer storage slots
3. Structs can be packed into fewer storage slots
4. Using bools for storage incurs overhead
5. Functions guaranteed to revert when called by normal users can be marked payable
6. Multiple accesses of a mapping/array should use a local variable cache
7. State variable should be cached inside the loop
8. IF’s/require() statements that check input arguments should be at the top of the function

QA

1. Array is push()ed but not pop()ed
2. Missing contract-existence checks before low-level calls
3. External call recipient may consume all transaction gas
4. Allowed fees/rates should be capped by smart contracts
5. Consider implementing two-step procedure for updating protocol addresses
6. safeApprove() is deprecated
7. Calls to withdraw() will revert when totalSupply() returns zero
8. Some tokens may revert when zero value transfers are made
9. Hardcoded treasuryFee may cause problem in future
10. Remove deprecated variables to avoid confusion

• Then deep dig to the documents thoroughly and understood protocols flow -Deep dive into code base with line by line analysis. The weak and vulnerable areas are marked with @audit tags
• Then analyzing each and every @audit tags more deep. Then performed all possible unit and fuzzing tests to ensured the functions are working indented way.
• Then i reported final reports as per C4 Formats

M/H Risks

1. Call function without checking status may lead to fund loss
2. Chainlink's latestRoundData return stale or incorrect result

Codebase quality analysis and Possible Risks Comments

OptionsPositionManager.sol

Code

1. In the executeOperation function, you can avoid the need to decode the params twice by using a local variable for the decoded mode value
2. Choose more descriptive variable names to improve code readability. For instance, replace amtA and amtB with more meaningful names that convey their purpose
3. The executeBuyOptions and executeLiquidation functions can be quite complex. Consider breaking down their functionality into smaller helper functions with well-defined purposes to improve code maintainability
4. Reduce nesting levels in functions such as closeDebt to enhance readability. Nested statements can become hard to follow, and breaking them down can help mitigate this issue
5. For repetitive actions like swapping tokens or interacting with the lending pool, consider centralizing common functionality into utility functions to reduce code duplication and promote consistency
6. Explicitly specify the visibility modifiers for functions to ensure clarity and to prevent accidental misuse
7. Replace hardcoded values like 1e18 or 1e8 with named constants or variables with explanatory names.
8. Provide detailed error messages in require statements to give better insight into why a condition failed and how to address the issue

Risks

• Ensure that only authorized users or contracts can call critical functions like liquidation
• The code may be susceptible to reentrancy attacks if external calls are made before ensuring the current execution context is secure. This could potentially allow malicious contracts to call back into your contract and manipulate its state.

OptionsPositionManager.sol

Code

1. Consider using the view or pure keywords for functions that don't modify state. This can help save gas when users query the contract
2. Use internal instead of public for returnExpectedBalanceWithoutFees: Since returnExpectedBalanceWithoutFees is intended to be used only within the contract, you can make it an internal function to prevent unnecessary external visibility
3. In the latestAnswer function, you can store the asset prices obtained from the Oracle in variables to avoid making duplicate calls
4. In the getTokenAmounts function, consider reusing the getTokenAmountsExcludingFees function instead of duplicating its logic
5. The code contains multiple conditional statements, calculations, and interactions with external contracts. This complexity could make the code harder to understand, debug, and maintain
6. Some parts of the code lack explanatory comments, which could make it challenging for other developers to understand the intentions and assumptions behind specific calculations.
7. The naming conventions for functions and variables are inconsistent, which could lead to confusion and misunderstandings among developers

Risks

• The contract relies on external price data for asset tokens (TOKEN0_PRICE and TOKEN1_PRICE) from an oracle. Inaccurate or manipulated price data could lead to incorrect fee calculations, affecting user balances and overall contract behavior
• The calculation of feeLiquidity and the subsequent distribution of fees may have precision and rounding issues. This could impact fee accuracy and user expectations when interacting with the contract
• The nonReentrant modifier is used, but it's important to ensure that external contract calls, such as POS_MGR.increaseLiquidity and POS_MGR.decreaseLiquidity, are placed after state changes to prevent reentrancy vulnerabilities
• The contract relies on external contracts (e.g., Uniswap V3 contracts) for key operations. Changes or upgrades to these contracts could impact the behavior and reliability of this contract
• External contract interactions, such as with Uniswap V3, could expose the contract to front-running attacks, where malicious actors manipulate transactions to their advantage
• While the contract checks minimum withdrawal amounts (amount0Min and amount1Min), it doesn't validate that these values are within reasonable ranges or handle potential edge cases
• There's no mechanism to handle cases where liquidity is removed from the contract, potentially resulting in stranded liquidity or unexpected contract behavior

RangeManager.sol

Code

1. Instead of using if statements followed by revert, you can use the require statement directly, which provides a more concise and readable way to check conditions and revert if they're not met
2. If the ranges have different states (e.g., active, closed), consider using an enum to define these states and track them in the Step struct
3. Consider using more compact data types when possible to reduce storage costs. For example, if the range IDs or step IDs won't exceed a certain limit, you can use smaller integer types like uint32 instead of uint256
4. Consider using the try / catch pattern for handling external contract calls to gracefully handle failures and avoid stopping contract execution completely

Risks

• The contract interacts with tokens using safeIncreaseAllowance. Ensure that you manage token approvals properly to prevent unauthorized transfers and potential attacks
• The checkNewRange function checks for overlaps between new ranges and existing ones. If there's a bug in this logic, it could lead to range overlaps, potentially causing unintended behavior
• The contract interacts with various external contracts, including the Aave lending pool, Uniswap, and more. Be aware that external contracts can change behavior, be upgraded, or become malicious, which might impact your contract's operation
• Transactions and state changes on the blockchain are visible to the public. If there are economically significant operations that can be front-run, malicious actors might take advantage of them
• Depending on the interaction with the Aave lending pool and Uniswap, there might be risks associated with the security and liquidity of these pools

PositionManager.sol

Code

1. Some functions lack visibility specifiers (public, internal, external, private), which can lead to confusion and unexpected behavior. Always specify the intended visibility of functions
2. Unused import statements, such as IUniswapV2Pair, IUniswapV2Factory, etc., suggest that the contract is importing unnecessary interfaces, which can increase deployment gas costs.
3. State variables like LENDING_POOL and ADDRESSES_PROVIDER are declared but not initialized in the constructor, which can lead to unexpected behaviour
4. Constructor parameters are not checked for validity, such as ensuring that the roerouter address is not address(0)
5. Arrays like tokenisedRanges and tokenisedTicker are maintained even though their elements might be removed. This can lead to inefficient use of storage and increase gas costs

Risks

• There are instances where variables are declared but not used. This could indicate unnecessary complexity or potential issues with logic
• The usage of type(uint256).max for setting allowances can be risky, as it may exceed the available gas limit in a transaction. It's better to set allowances to a reasonable value
• The cleanup function doesn't handle the case where repay or deposit calls fail, which can result in partial or failed cleanup operations.
• The direct use of .balanceOf() on an ERC20 token can cause issues if the token doesn't implement the ERC20 standard properly. It's safer to use the SafeERC20 library
• In the validateValuesAgainstOracle function, the check for value comparisons might not be accurate due to precision issues with decimals
• In the checkNewRange function, when detecting overlapping ranges, the contract immediately reverts. This doesn't provide any way to handle overlapping ranges, potentially leading to failed transactions and user confusion
• In the cleanup function, you're calling the repay and deposit functions of the lending pool without checking their return values. If these operations fail, the contract won't handle it, potentially leading to funds getting stuck
• Ensure that the arithmetic operations involving token amounts and prices are performed with correct precision to prevent unintended consequences due to rounding errors

Gas Optimization

This analysis report is meant to accompany my Gas Optimizations Report submission. All insights will be given through the perspective of optimizing the codebase

1. By utilizing calldata instead of memory arrays, the contract avoids the gas-intensive abi.decode() loop for array copying. This saves gas by bypassing the need for individual index copying.
2. Optimally packing state variables within 32-byte storage slots efficiently uses storage space. Fewer slots lead to less SLOAD usage, resulting in substantial gas savings
3. Efficiently packing structs into fewer storage slots avoids extra SLOAD operations. Using smaller types, like uint96, further reduces storage costs and saves gas
4. Replacing bools with uint256 (1 or 2) bypasses extra SLOADs, while marking guaranteed-to-revert functions as payable reduces gas costs by avoiding unnecessary checks
5. Caching mapping and array accesses in local variables saves gas by eliminating recalculations of key hashes and offsets for each access in the loop
6. Caching state variables inside loops reduces gas costs by minimizing storage slot accesses, improving execution efficiency.
7. Moving constant checks and input argument validations to the top of functions minimizes gas costs for invalid input by failing quickly with low gas costs.
8. When functions are equipped with a modifier like onlyOwner, they automatically revert if regular users attempt to make calls. Marking these functions as payable lowers gas costs for legitimate callers since the compiler omits payment checks. By avoiding extra opcodes like CALLVALUE, DUP1, ISZERO, PUSH2, JUMPI, PUSH1, DUP1, REVERT, JUMPDEST, and POP, each call saves around 21 gas on average. Additionally, this optimization reduces deployment costs

These optimizations, backed by opcode explanations, enhance the contract's gas efficiency and overall performance

Centralization risks

The "onlyowner" keyword in smart contracts allows a single address to have complete control over the contract. This can lead to centralization risks, as the single owner could potentially abuse their power

The single owner could:

1. Freeze the funds of other users.
2. Change the rules of the contract.
3. Terminate the contract.

101:   function setEnabled(bool _isEnabled) public onlyOwner  

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

108:   function setTreasury(address newTreasury) public onlyOwner  

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

116:   function pushTick(address tr) public onlyOwner  

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

137:   function shiftTick(address tr) public onlyOwner  

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

167:   function modifyTick(address tr, uint index) public onlyOwner  

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

183:   function setBaseFee(uint newBaseFeeX4) public onlyOwner  

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

191:   function setTvlCap(uint newTvlCap) public onlyOwner  

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

94:     function emergencyWithdraw(ERC20 token) onlyOwner external  

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/helper/V3Proxy.sol#L94-L94

75:   function generateRange(uint128 startX10, uint128 endX10, string memory startName, string memory endName, address beacon) external onlyOwner  

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/RangeManager.sol#L75-L75

95:   function initRange(address tr, uint amount0, uint amount1) external onlyOwner 

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/RangeManager.sol#L95-L95

48:   function deprecatePool(uint poolId) public onlyOwner 

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/RoeRouter.sol#L48-L48

59:   function addPool(
60:     address lendingPoolAddressProvider, 
61:     address token0, 
62:     address token1, 
63:     address ammRouter
64:   ) 
65:     public onlyOwner 
66:     returns (uint poolId)
67:   

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/RoeRouter.sol#L65-L65

24:     function setHardcodedPrice(int256 _hardcodedPrice) external onlyOwner 

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/helper/FixedOracle.sol#L24-L24

Time Spend

10 hours

Time spent:

10 hours