Omniscia Gamma Strategies Audit

GammaController Manual Review Findings

GammaController Manual Review Findings

GCR-01M: Inexistent Approval Mechanism

Description:

The TokeHypervisor contract's withdraw implementation utilizes no concept of an approval and as such can be invoked by arbitrary parties.

Example:

contracts/adapters/tokemak/GammaController.sol
56/// @notice Withdraw liquidity from Hypervisor ( controller owns LP tokens, controller receives assets ) 
57/// @dev Calls to external contract
58/// @dev We trust sender to send a true gamma lpTokenAddress. If it's not the case it will fail in the UniProxy deposit require.
59/// @param lpTokenAddress LP Token(Hypervisor) address
60/// @param amount Quantity of LP tokens to burn in the withdrawal
61function withdraw(
62    address lpTokenAddress,
63    uint256 amount,
64    uint256[N_COINS] memory minAmounts
65) external onlyManager {
66    
67    uint256 lpTokenBalanceBefore = IERC20(lpTokenAddress).balanceOf(manager);
68    uint256[N_COINS] memory coinsBalancesBefore = _getCoinsBalances(lpTokenAddress);
69

70    ITokeHypervisor(lpTokenAddress).withdraw(amount, manager, manager);
71

72    uint256 lpTokenBalanceAfter = IERC20(lpTokenAddress).balanceOf(manager);
73    uint256[N_COINS] memory coinsBalancesAfter = _getCoinsBalances(lpTokenAddress);
74

75    _compareCoinsBalances(coinsBalancesBefore, coinsBalancesAfter, minAmounts);
76

77    require(lpTokenBalanceBefore - amount == lpTokenBalanceAfter, "LP_TOKEN_MISMATCH");
78}

Recommendation:

We advise this trait of the system to be carefully evaluated and if incorrect the canonical Hypervisor implementation to be introduced to the codebase to properly assess vulnerabilities such as the race condition of a deposit operation.

Alleviation:

The withdraw function within the Hypervisor now properly enforces access control by ensuring that the caller is whitelisted if a whitelist is toggled on, thereby alleviating this exhibit.

GCR-02M: Race-Condition Prone Hypervisor Integration

Description:

The GammaController contract will invoke the Hypervisor with a from argument that is different than the contract itself. This implies that an allowance has been set by the manager to the Hypervisor prior to the execution of the deploy function. If this does not occur atomically in a single transaction, the code is fully susceptible to a race-condition whereby a bot detects the Hypervisor liquidity provision transaction via the GammaController and invokes deposit with a different to argument extracting the approved funds from the manager and acquiring the LP tokens for themselves.

Example:

contracts/adapters/tokemak/GammaController.sol
24/// @notice Deploy liquidity to a Gamma Hypervisor ( controller owns assets, manager receives LP tokens )
25 /// @dev Calls to external contract
26 /// @dev We trust sender to send a true gamma lpTokenAddress
27 /// @param amount0 quantity of token0 of Hypervisor 
28 /// @param amount1 quantity of token1 of Hypervisor 
29 /// @param lpTokenAddress LP Token(Hypervisor) address
30 function deploy(
31   uint256 amount0,
32   uint256 amount1,
33   address lpTokenAddress,
34uint256 minMintAmount
35 ) external onlyManager {
36

37     uint256 balance0 = ITokeHypervisor(lpTokenAddress).token0().balanceOf(manager);
38     uint256 balance1 = ITokeHypervisor(lpTokenAddress).token1().balanceOf(manager);
39

40     require(balance0 >= amount0 && balance1 >= amount1, "INSUFFICIENT_BALANCE");
41

42     // approve Hypervisor to spend amount0,1 amounts of Hypervisor.token0,1
43     _approve(ITokeHypervisor(lpTokenAddress).token0(), lpTokenAddress, amount0);
44     _approve(ITokeHypervisor(lpTokenAddress).token1(), lpTokenAddress, amount1);
45

46     uint256 lpTokenBalanceBefore = IERC20(lpTokenAddress).balanceOf(manager);
47     // deposit amount0, amount1 and mint LP tokens to the manager 
48     // uint256 lpTokenReceived = uniProxy.deposit(amount0, amount1, manager, lpTokenAddress);
49     uint256 lpTokenReceived = ITokeHypervisor(lpTokenAddress).deposit(amount0, amount1, manager, manager);
50

51     uint256 lpTokenBalanceAfter = IERC20(lpTokenAddress).balanceOf(manager);
52     require(lpTokenBalanceBefore + lpTokenReceived == lpTokenBalanceAfter, "LP_TOKEN_MISMATCH");
53    require(lpTokenReceived >= minMintAmount, "INSUFFICIENT_MINT");
54 }

Recommendation:

We advise this trait of the system to be considered and potentially refactored as in its current implementation it will be exploited by front-running attacks.

Alleviation:

After extensive discussions with the Gamma Controller team and sufficient documentation provided on their end, we concluded that this particular attack vector is inapplicable in their envisioned production environment given the fact that the deploy function of GammaController applies a level of access control and the deploy function of the Tokemak instance will have the whitelist toggled on with a single user capable of invoking the function. As a combination of the aforementioned, we consider this exhibit nullified.

GCR-03M: Inexistent Validation of Hypervisor Address

Description:

The GammaController system has been built to blindly trust the lpTokenAddress argument passed to it.

Example:

contracts/adapters/tokemak/GammaController.sol
24/// @notice Deploy liquidity to a Gamma Hypervisor ( controller owns assets, manager receives LP tokens )
25 /// @dev Calls to external contract
26 /// @dev We trust sender to send a true gamma lpTokenAddress
27 /// @param amount0 quantity of token0 of Hypervisor 
28 /// @param amount1 quantity of token1 of Hypervisor 
29 /// @param lpTokenAddress LP Token(Hypervisor) address
30 function deploy(
31   uint256 amount0,
32   uint256 amount1,
33   address lpTokenAddress,
34uint256 minMintAmount
35 ) external onlyManager {
36

37     uint256 balance0 = ITokeHypervisor(lpTokenAddress).token0().balanceOf(manager);
38     uint256 balance1 = ITokeHypervisor(lpTokenAddress).token1().balanceOf(manager);

Recommendation:

We advise a lookup operation to be performed instead on the HypervisorFactory based on the token0 and token1 arguments as well as an additional fee argument passed in to the functions. This will significantly optimize the gas cost of the function as the token0 and token1 variables will be used without external calls and it will also ensure more consistent user experience as the user's will know exactly what tokens they are providing with and the prospect of a malicious Hypervisor implementation will be eliminated completely.

Alleviation:

The getHypervisor function is now utilized by the factory thus optimizing its gas cost and ensuring no check is necessary.

GCR-04M: Superficial Balance Increment Evaluation

Description:

The linked code attempts to ascertain proper withdrawal integration with the Hypervisor but does so by simply validating that the balance of the contract increased and does not actually validate the increase amount.

Example:

contracts/adapters/tokemak/GammaController.sol
91function _compareCoinsBalances(uint256[N_COINS] memory balancesBefore, uint256[N_COINS] memory balancesAfter, uint256[N_COINS] memory amounts) internal {
92    for (uint256 i = 0; i < N_COINS; i++) {
93        if (amounts[i] > 0) {
94            require(balancesBefore[i] < balancesAfter[i], "BALANCE_MUST_INCREASE");
95        }
96    }
97}

Recommendation:

We advise the validations to be performed more stringently by ensuring that the delta is at least the minimum amount specified in the withdrawal operation.

Alleviation:

The code now correctly evaluates that the delta satisfies the minimum set by the caller during the function's execution thereby alleviating this exhibit in full.

GCR-05M: Withdrawal Documentation Mismatch

Description:

The documentation of the project states that the controller is meant to receive the assets in a withdrawal, however, the manager ultimately receives them.

Example:

contracts/adapters/tokemak/GammaController.sol
56/// @notice Withdraw liquidity from Hypervisor ( controller owns LP tokens, controller receives assets ) 
57/// @dev Calls to external contract
58/// @dev We trust sender to send a true gamma lpTokenAddress. If it's not the case it will fail in the UniProxy deposit require.
59/// @param lpTokenAddress LP Token(Hypervisor) address
60/// @param amount Quantity of LP tokens to burn in the withdrawal
61function withdraw(
62    address lpTokenAddress,
63    uint256 amount,
64    uint256[N_COINS] memory minAmounts
65) external onlyManager {
66    
67    uint256 lpTokenBalanceBefore = IERC20(lpTokenAddress).balanceOf(manager);
68    uint256[N_COINS] memory coinsBalancesBefore = _getCoinsBalances(lpTokenAddress);
69

70    ITokeHypervisor(lpTokenAddress).withdraw(amount, manager, manager);

Recommendation:

We advise this to be corrected in the documentation as this is logical behaviour within the contract.

Alleviation:

The Gamma Strategies team considered this exhibit but opted not to apply a remediation for it in the current iteration of the codebase.

Prev

GammaController.sol (GCR-S)

Next

BaseController.sol (BCR-C)

On this page