Gamma

Description

Hypervisor.deposit pulls pre-approved ERC20 tokens from the from address to the contract. Later it mints shares to the to address. Attackers can determine both the from and to addresses as they wish, and thus steal shares (that can be redeemed to tokens immediately) from users that pre-approved the contract to spend ERC20 tokens on their behalf.

Recommendation

As described in issue 3.5, we recommend restricting access to this function only for UniProxy. Moreover, the UniProxy contract should validate that from == msg.sender.

Description

the call to Router.exactInputrequires the sender to pre-approve the tokens. We could not find any reference for that, thus we assume that a call to UniProxy.depositSwap will always revert.

Examples

code/contracts/UniProxy.sol:L202-L234

router = ISwapRouter(_router);
uint256 amountOut;
uint256 swap;
if(swapAmount < 0) {
    //swap token1 for token0

    swap = uint256(swapAmount * -1);
    IHypervisor(pos).token1().transferFrom(msg.sender, address(this), deposit1+swap);
    amountOut = router.exactInput(
        ISwapRouter.ExactInputParams(
            path,
            address(this),
            block.timestamp + swapLife,
            swap,
            deposit0
        )
    );
}
else{
    //swap token1 for token0
    swap = uint256(swapAmount);
    IHypervisor(pos).token0().transferFrom(msg.sender, address(this), deposit0+swap);

    amountOut = router.exactInput(
        ISwapRouter.ExactInputParams(
            path,
            address(this),
            block.timestamp + swapLife,
            swap,
            deposit1
        )
    );     
}

Recommendation

Consider approving the exact amount of input tokens before the swap.

Description

Uniproxy.depositSwap uses _router that is determined by the caller, which in turn might inject a “fake” contract, and thus may steal funds stuck in the UniProxy contract.

The UniProxy contract has certain trust assumptions regarding the router. The router is supposed to return not less than deposit1(or deposit0) amount of tokens but that fact is never checked.

Examples

code/contracts/UniProxy.sol:L168-L177

function depositSwap(
  int256 swapAmount, // (-) token1, (+) token0 for token1; amount to swap
  uint256 deposit0,
  uint256 deposit1,
  address to,
  address from,
  bytes memory path,
  address pos,
  address _router
) external returns (uint256 shares) {

Recommendation

Consider removing the _router parameter from the function, and instead, use a storage variable that will be initialized in the constructor.

Description

The UniProxy contract has a price manipulation protection:

code/contracts/UniProxy.sol:L75-L82

if (twapCheck || positions[pos].twapOverride) {
  // check twap
  checkPriceChange(
    pos,
    (positions[pos].twapOverride ? positions[pos].twapInterval : twapInterval),
    (positions[pos].twapOverride ? positions[pos].priceThreshold : priceThreshold)
  );
}

But after that, the tokens are transferred from the user, if the token transfer allows an attacker to hijack the call-flow of the transaction inside, the attacker can manipulate the Uniswap price there, after the check happened. The Hypervisor’s deposit function itself is vulnerable to the flash-loan attack.

Recommendation

Make sure the price does not change before the Hypervisor.deposit call. For example, the token transfers can be made at the beginning of the UniProxy.deposit function.

Description

The deposit function is designed to be called only from the UniProxy contract, but everyone can call it. This function does not have any protection against price manipulation in the Uniswap pair. A deposit can be frontrunned, and the depositor’s funds may be “stolen”.

Recommendation

Make sure only UniProxy can call the deposit function.

Description

UniProxy.properDepositRatio purpose is to be used as a mechanism to prevent liquidity imbalance. The idea is to compare the deposit ratio with the hypeRatio, which is the ratio between the tokens held by the Hypervisor contract. In practice, however, this function will not prevent a skewed deposit ratio in many cases. deposit1 / deposit0 might be a huge number, while 10^16 <= depositRatio <= 10^18, and 10^16 <= hypeRatio <= 10^18. Let us consider the case where hype1 / hype0 >= 10, that means hypeRatio = 10^18, and now if deposit1 / deposit0 = 10^200 for example, depositRatio = 10^18, and the transaction will pass, which is clearly not intended.

Examples

code/contracts/UniProxy.sol:L258-L275

function properDepositRatio(
  address pos,
  uint256 deposit0,
  uint256 deposit1
) public view returns (bool) {
  (uint256 hype0, uint256 hype1) = IHypervisor(pos).getTotalAmounts();
  if (IHypervisor(pos).totalSupply() != 0) {
    uint256 depositRatio = deposit0 == 0 ? 10e18 : deposit1.mul(1e18).div(deposit0);
    depositRatio = depositRatio > 10e18 ? 10e18 : depositRatio;
    depositRatio = depositRatio < 10e16 ? 10e16 : depositRatio;
    uint256 hypeRatio = hype0 == 0 ? 10e18 : hype1.mul(1e18).div(hype0);
    hypeRatio = hypeRatio > 10e18 ? 10e18 : hypeRatio;
    hypeRatio = hypeRatio < 10e16 ? 10e16 : hypeRatio;
    return (FullMath.mulDiv(depositRatio, deltaScale, hypeRatio) < depositDelta &&
            FullMath.mulDiv(hypeRatio, deltaScale, depositRatio) < depositDelta);
  }
  return true;
}

Recommendation

Consider removing the cap of [0.1,10] both for depositRatio and for hypeRatio.

Description

The UniProxy contract declares the usage of the SafeERC20 library for functions of the IERC20 type. However, unsafe functions are used instead of safe ones.

Examples

• Usage of approve instead of safeApprove
• Usage of transferFrom instead of safeTransferFrom.

Examples

1. The UniProxy contract has different functions used for setting the properties of a position. However, Position.priceThreshold, and Position.depositOverride are never assigned to, even though they are being used.

2. UniProxy.deposit is calling IHypervisor.deposit multiple times with different function signatures (3 and 4 parameters), while the Hypervisor contract only implements the version with 4 parameters, and does not implement the IHypervisor interface.

3. Hypervisor.uniswapV3MintCallback | uniswapV3SwapCallback - both these functions contain unreachable code, namely the case where payer != address(this).

Recommendations

1. Consider adding functions to set these properties, or alternatively, a single function to set the properties of a position.
2. Consider supporting a single deposit function for IHypervisor, and make sure that the actual implementation adheres to this interface.
3. Consider deleting these lines.

Description

Hypervisor.withdraw can be used by a liquidity provider to withdraw its deposit from the Hypervisor contract. A user can get his deposited liquidity back in exchange for the burn of his shares. The function is transferring token0,1 to the user first and then burns his shares. In theory, the contracts of token0,1 may hijack the execution call-flow causing a reentrant call to deposit, which will use the stale value for totalSupply() to evaluate the number of shares to be minted. Since this value will be greater than what it should be, the attacker will be able to mint shares for free, that could be later redeemed for actual tokens stolen from other depositors.

Recommendation

Consider adding a ReentrancyGuard both to Hypervisor.withdraw and Hypervisor.deposit

Description

When executing the swap, the minimal amount out is passed to the router (deposit1 in this example), but the actual swap amount will be amountOut. But after the trade, instead of depositing amountOut, the contract tries to deposit deposit1, which is lower. This may result in some users’ funds staying in the UniProxy contract.

code/contracts/UniProxy.sol:L220-L242

else{
    //swap token1 for token0
    swap = uint256(swapAmount);
    IHypervisor(pos).token0().transferFrom(msg.sender, address(this), deposit0+swap);

    amountOut = router.exactInput(
        ISwapRouter.ExactInputParams(
            path,
            address(this),
            block.timestamp + swapLife,
            swap,
            deposit1
        )
    );     
}

require(amountOut > 0, "Swap failed");

if (positions[pos].version < 2) {
  // requires lp token transfer from proxy to msg.sender
  shares = IHypervisor(pos).deposit(deposit0, deposit1, address(this));
  IHypervisor(pos).transfer(to, shares);
}

Recommendation

Deposit all the user’s funds to the Hypervisor.

Description

The amount of tokens received from UniswapV3Pool functions might be manipulated by front-runners due to the decentralized nature of AMMs, where the order of transactions can not be pre-determined. A potential “sandwicher” may insert a buying order before the user’s call to Hypervisor.rebalance for instance, and a sell order after.

More specifically, calls to pool.swap, pool.mint, pool.burn are susceptible to “sandwiching” vectors.

Examples

Hypervisor.rebalance

code/contracts/Hypervisor.sol:L278-L286

if (swapQuantity != 0) {
    pool.swap(
        address(this),
        swapQuantity > 0,
        swapQuantity > 0 ? swapQuantity : -swapQuantity,
        swapQuantity > 0 ? TickMath.MIN_SQRT_RATIO + 1 : TickMath.MAX_SQRT_RATIO - 1,
        abi.encode(address(this))
    );
}

code/contracts/Hypervisor.sol:L348-L363

function _mintLiquidity(
    int24 tickLower,
    int24 tickUpper,
    uint128 liquidity,
    address payer
) internal returns (uint256 amount0, uint256 amount1) {
    if (liquidity > 0) {
        (amount0, amount1) = pool.mint(
            address(this),
            tickLower,
            tickUpper,
            liquidity,
            abi.encode(payer)
        );
    }
}

code/contracts/Hypervisor.sol:L365-L383

function _burnLiquidity(
    int24 tickLower,
    int24 tickUpper,
    uint128 liquidity,
    address to,
    bool collectAll
) internal returns (uint256 amount0, uint256 amount1) {
    if (liquidity > 0) {
        // Burn liquidity
        (uint256 owed0, uint256 owed1) = pool.burn(tickLower, tickUpper, liquidity);

        // Collect amount owed
        uint128 collect0 = collectAll ? type(uint128).max : _uint128Safe(owed0);
        uint128 collect1 = collectAll ? type(uint128).max : _uint128Safe(owed1);
        if (collect0 > 0 || collect1 > 0) {
            (amount0, amount1) = pool.collect(to, tickLower, tickUpper, collect0, collect1);
        }
    }
}

Recommendation

Consider adding an amountMin parameter(s) to ensure that at least the amountMin of tokens was received.

Description

The test suite at this stage is not complete. It is crucial to have a full test coverage that includes the edge cases and failure scenarios, especially for complex system like Gamma.

As we’ve seen in some smart contract incidents, a complete test suite can prevent issues that might be hard to find with manual reviews.

Some issues such as issue 3.8, issue 3.2 could be caught by a full-coverage test suite.

Description

Uniswap v3 uses a callback pattern to pull funds from the caller. The caller, (in this case Hypervisor) has to implement a callback function which will be called by the Uniswap’s pool. Both uniswapV3MintCallback and uniswapV3SwapCallback restrict the access to the callback functions only for the pool. However, this alone will not block a random call from the pool contract in case the latter was hacked, which will result in stealing all the funds held in Hypervisor or of any user that approved the Hypervisor contract to transfer tokens on his behalf.

Examples

code/contracts/Hypervisor.sol:L407-L445

function uniswapV3MintCallback(
    uint256 amount0,
    uint256 amount1,
    bytes calldata data
) external override {
    require(msg.sender == address(pool));
    address payer = abi.decode(data, (address));

    if (payer == address(this)) {
        if (amount0 > 0) token0.safeTransfer(msg.sender, amount0);
        if (amount1 > 0) token1.safeTransfer(msg.sender, amount1);
    } else {
        if (amount0 > 0) token0.safeTransferFrom(payer, msg.sender, amount0);
        if (amount1 > 0) token1.safeTransferFrom(payer, msg.sender, amount1);
    }
}

function uniswapV3SwapCallback(
    int256 amount0Delta,
    int256 amount1Delta,
    bytes calldata data
) external override {
    require(msg.sender == address(pool));
    address payer = abi.decode(data, (address));

    if (amount0Delta > 0) {
        if (payer == address(this)) {
            token0.safeTransfer(msg.sender, uint256(amount0Delta));
        } else {
            token0.safeTransferFrom(payer, msg.sender, uint256(amount0Delta));
        }
    } else if (amount1Delta > 0) {
        if (payer == address(this)) {
            token1.safeTransfer(msg.sender, uint256(amount1Delta));
        } else {
            token1.safeTransferFrom(payer, msg.sender, uint256(amount1Delta));
        }
    }
}

Recommendation

Consider adding (boolean) storage variables that will help to track whether a call to uniswapV3MintCallback | uniswapV3SwapCallback was preceded by a call to _mintLiquidity | rebalance respectively. An example for the rebalance function would be bool rebalanceCalled, this variable will be assigned a true value in rebalance before the external call of pool.swap, then uniswapV3SwapCallback will require that rebalanceCalled == true, and then right after rebalanceCalled will be assigned a false value.

Examples

1. UniProxy.deposit - from parameter is never used.
2. UniProxy - MAX_INT should be changed to MAX_UINT.
3. Consider using compiler version >= 0.8.0, and make sure that the compiler version is specified explicitly for every .sol file in the repo.
4. UniProxy - Minimize code duplication in deposit and depositSwap.