Core, Strategies and Periphery - GammaSwap Labs

The invariant factor formula makes use of the number of decimals of the first token in token pair. The result of this calculation is further used in multiple places across the contracts. For example, when you borrow liquidity from a GammaSwap pool, the protocol calculates that factor to update some storage variables such as lastFeeIndex and BORROWED_INVARIANT. Instead of focusing on s.decimal[0] only, both decimals should be considered.

As a result, calculating this variable based on a wrong number of decimals affects borrowed/repaid liquidity and many storage variables in the protocol.

Invariant Factor PoC

// replace the s.decimals[0] variable in the getInvariantFactor() with 1e18 and 1e6 to see difference.
function test_BorrowAndRepayLifeCycleTask01() public {
    vm.roll(16392000 + 1200);
    _addLiquidityWithUniRouter(deployer, address(usdt), address(weth), 10 * 1e6, 5 * 1e18);

    vm.startPrank(deployer);

    usdt.transfer(user1, 5 * 1e6);
    weth.transfer(user1, 2 * 1e18);

    usdt.transfer(user2, 6 * 1e6);
    weth.transfer(user2, 10 * 1e18);


    uint256 loanId = positionManager.createLoan(1, address(cfmm_usdt_weth), deployer, type(uint).max);
    uint256 loanId2 = positionManager.createLoan(1, address(cfmm_usdt_weth), user2, type(uint).max);
    positionManager.transferFrom(deployer, user1, loanId);

    IPositionManager.DepositWithdrawParams memory depositData = IPositionManager.DepositWithdrawParams({
        protocolId: 1,
        cfmm: address(cfmm_usdt_weth),
        to: user1,
        lpTokens: cfmm_usdt_weth.balanceOf(deployer),
        deadline: 99999
    });

    cfmm_usdt_weth.approve(address(positionManager), cfmm_usdt_weth.balanceOf(deployer));

    positionManager.depositNoPull(depositData);

    vm.stopPrank();

    _addLiquidityWithUniRouter(user1, address(usdt), address(weth), 1e6, 1e18);

    vm.startPrank(user1);

    uint256[] memory amountsDesired = new uint256[](2);
    amountsDesired[0] = 1e18;
    amountsDesired[1] = 1e6;

    IPositionManager.AddRemoveCollateralParams memory increaseCollData = IPositionManager.AddRemoveCollateralParams({
        protocolId: 1,
        cfmm: address(cfmm_usdt_weth),
        to: user1,
        tokenId: loanId,
        deadline: 99999,
        amounts: amountsDesired
    });

    usdt.approve(address(positionManager), amountsDesired[1]); 
    weth.approve(address(positionManager), amountsDesired[0]);

    vm.roll(16392000 + 1252);
    positionManager.increaseCollateral(increaseCollData);

    vm.stopPrank();

    vm.startPrank(user2);

    increaseCollData.tokenId = loanId2;
    increaseCollData.to = user2;

    amountsDesired[0] = 1e18;
    amountsDesired[1] = 1e6;

    increaseCollData.amounts = amountsDesired;

    usdt.approve(address(positionManager), amountsDesired[1]); 
    weth.approve(address(positionManager), amountsDesired[0]);

    vm.roll(16392000 + 1304);
    positionManager.increaseCollateral(increaseCollData);

    vm.stopPrank();

    vm.startPrank(user1);

    IPositionManager.BorrowLiquidityParams memory borrowLqtyData = IPositionManager.BorrowLiquidityParams({
        protocolId: 1,
        cfmm: address(cfmm_usdt_weth),
        to: user1,
        tokenId: loanId,
        lpTokens: uint256(1e12) * 800 / 1000,
        deadline: 99999,
        minBorrowed: new uint256[](2)
    });

    positionManager.borrowLiquidity(borrowLqtyData);

    vm.roll(16392000 + 1340);

    amountsDesired[0] = 1e18;
    amountsDesired[1] = 1e6;

    increaseCollData.amounts = amountsDesired;
    vm.stopPrank();

    vm.prank(user2);
    positionManager.decreaseCollateral(increaseCollData);


    IPositionManager.RepayLiquidityParams memory repayData = IPositionManager.RepayLiquidityParams({
        protocolId: 1,
        cfmm: address(cfmm_usdt_weth),
        to: user1,
        tokenId: loanId,
        liquidity: 1000000 / 2,
        deadline: 99999,
        minRepaid: new uint256[](2)
    });

    vm.stopPrank();

    vm.startPrank(deployer);
    usdt.transfer(address(cfmm_usdt_weth), 10 * 1e6);
    weth.transfer(address(cfmm_usdt_weth), 5 * 1e18);
    vm.stopPrank();
}

Screenshot:

Code Location

BaseStrategy

BaseStrategy.sol

function getInvariantFactor() internal virtual override view returns(uint256) {
    return 10 ** s.decimals[0];
}

AbstractRateModel

AbstractRateModel.sol

function calcUtilizationRate(uint256 lpInvariant, uint256 borrowedInvariant) internal virtual view returns(uint256) {
    uint256 totalInvariant = lpInvariant + borrowedInvariant;
    if(totalInvariant == 0)
        return 0;

    return borrowedInvariant * getInvariantFactor() / totalInvariant;
}

SOLVED: This finding was identified in a code walkthrough jointly by the GammaSwap team and the Halborn team, and the existence of the finding was confirmed by the Halborn team. The invariant factor is now calculated as 10**18.

Commit ID: v1-strategies::85864193924b7d1299dd99a27ded752c807ae2cb

The _batchLiquidations function implemented in the LiquidationStrategy contract is designed to perform more than one liquidation in a go. The tokenId > 0 check on the payLoanAndRefundLiquidator function assures that users can use 0 as tokenId for batch liquidation operation. However, it is not possible to use 0 as tokenId in the _batchLiquidations function.

The _batchLiquidations function makes an internal call to the sumLiquidity function. During the calculation of the liquidity variable, the execution is reverted with the Division or modulo by 0 error since the _loan.rateIndex is also zero for tokenId 0.

Code Location

LiquidationStrategy.sol

function sumLiquidity(uint256[] calldata tokenIds) internal virtual returns(uint256 liquidityTotal, uint256 collateralTotal, uint256 lpTokensPrincipalTotal, uint128[] memory tokensHeldTotal) {
    address[] memory tokens = s.tokens;
    uint128[] memory tokensHeld;
    address cfmm = s.cfmm;
    tokensHeldTotal = new uint128[](tokens.length);
    (uint256 accFeeIndex,,) = updateIndex();
    for(uint256 i = 0; i < tokenIds.length; i++) {
        LibStorage.Loan storage _loan = s.loans[tokenIds[i]];
        uint256 liquidity = uint128((_loan.liquidity * accFeeIndex) / _loan.rateIndex);
        tokensHeld = _loan.tokensHeld;
        lpTokensPrincipalTotal = lpTokensPrincipalTotal + _loan.lpTokens;
        _loan.liquidity = 0;
        _loan.initLiquidity = 0;
        _loan.rateIndex = 0;
        _loan.lpTokens = 0;
        uint256 collateral = calcInvariant(cfmm, tokensHeld);
        canLiquidate(collateral, liquidity, 950);
        collateralTotal = collateralTotal + collateral;
        liquidityTotal = liquidityTotal + liquidity;
        for(uint256 j = 0; j < tokens.length; j++) {
            tokensHeldTotal[j] = tokensHeldTotal[j] + tokensHeld[j];
            _loan.tokensHeld[j] = 0;
        }
    }
}

NOT APPLICABLE: The GammaSwap team confirmed that throwing the "Division or modulo by zero" error is the intended behavior.

The extra condition in the if statement described in the Code Location section poses a centralization risk. The isRestricted function is designed to check if a protocolId is restricted. However, the _owner of the contract is excluded from this control. This increases the centralization of the contract.

Code Location

GammaPoolFactory

GammaPoolFactory.sol

function isRestricted(uint16 protocolId, address _owner) internal virtual view {
    if(isProtocolRestricted[protocolId] == true && msg.sender != _owner) {
        revert ProtocolRestricted();
    }
}

RISK ACCEPTED: The GammaSwap team accepted the risk, and they confirmed a multisig wallet will be the owner of the contract.

There is an edge case scenario of token swapping operation which leads to the division by zero error. The beforeSwapTokens function in the CPMMBaseLongStrategy contract is a function which calculates the exact in and out amounts of the swap operation. If amountIn and reserveIn parameters of calcAmtOut() function are equal, then the denominator is equal to zero. As a result, the swapping operation fails in this case.

Code Location

CPMMBaseLongStrategy.sol

function calcAmtOut(uint256 amountIn, uint256 reserveOut, uint256 reserveIn) internal view returns (uint256) {
    if(reserveOut == 0 || reserveIn == 0) {
        revert ZeroReserves();
    }
    uint256 denominator = (reserveIn - amountIn) * tradingFee1;
    return (reserveOut * amountIn * tradingFee2 / denominator) + 1;
}

NOT APPLICABLE: The GammaSwap team confirmed the delta of reserveIn and amountIn will not be equal to 0 for Uniswap swaps. Therefore, reaching the "Division by zero" revert is impossible in this edge case.

Contracts in-scope are missing address validation in constructors and setter functions. It is possible to configure the address(0), which may cause issues during execution.

For instance, if address(0) is passed to setFeeToSetter function, it will not be possible to change this address in the future.

Code Location

Following functions are not validating, that given address is different from zero:

• PositionManager.sol, #55 - payee
• GammaPoolFactory.sol, #95 - setFeeToSetter
• ShortStrategy.sol, #36 - to
• ShortStrategy.sol, #68 - to
• BaseLongStrategy.sol, #34 - to

SOLVED: This finding was solved by the GammaSwap team by requiring the payee and setFeeToSetter variables not to be equal to the zero address in the PositionManager and GammaPoolFactory contracts.

• PositionManager - payee, fix:v1-periphery:f273ae...bfc50
• GammaPoolFactory - setFeeToSetter, fix:v1-core:0a735...5d623

This finding is not applicable to the (to) variables because the GammaSwap team will allow users to send their funds to address(0) in order to burn assets.

The _baseRate, _factor, _maxApy parameters of the LogDerivativeRateModel contract and _baseRate, _slope1, _slope2, _optimalUtilRate of the LinearKinkedRateModel contract are not checked to fall in the expected ranges. Some function calls might be therefore reverted due to possible arithmetic overflow issues, since no sanity checks are performed.

Code Location

LogDerivativeRateModel

LogDerivativeRateModel

constructor(uint64 _baseRate, uint80 _factor, uint80 _maxApy) {
    baseRate = _baseRate;
    factor = _factor;
    maxApy = _maxApy;
}

LinearKinkedRateModel

LinearKinkedRateModel

constructor(uint64 _baseRate, uint64 _optimalUtilRate, uint64 _slope1, uint64 _slope2) {
    baseRate = _baseRate;
    optimalUtilRate = _optimalUtilRate;
    slope1 = _slope1;
    slope2 = _slope2;
}

SOLVED: The GammaSwap team fixed this issue by adding sanity checks in the above contracts.

Commit ID: v1-strategies::db000bdedb35ce0c7c6e93f892f552c21be286d4

In for loops, the variable i is incremented using i++. It is known that in loops, using ++i costs less gas per iteration than i++.

It has also been detected that some uint256 variables are initialized to 0. These variables are already initialized to 0 by default, so uint256 i = 0 would reassign the 0 to i which wastes gas.

In addition, starting from pragma 0.8.0, adding unchecked keyword for arithmetical operations can reduce gas usage on contracts where underflow/underflow is unrealistic.

SOLVED: The GammaSwap team solved this issue by optimizing the for loops in contracts.

Commit IDs:

• v1-strategies::aba9e566ec55c61d349575b0c445ccb91d5fab39
• v1-core::f642d03fe55aeead926ccc1c3eb36c31d52d454d
• v1-periphery::fbbf9befe45a0818b35c1916d7eb6a7e5bceb606

The _name and _symbol variables in the GammaPoolERC721 contract were designed to be set only once. The immutable keyword consumes less gas. It might be useful to declare these variables as immutable to optimize gas usage in the protocol.

Code Location

GammaPoolERC721.sol

string private _name;

// Token symbol
string private _symbol;

SOLVED: This finding was solved by the GammaSwap team by moving the _name and _symbol variables to the PositionManager contract and declaring them constant.

Commit ID: v1-periphery::4e2b377de6888c1f7845cbd9485605ecfca053f1

There are a few unused imports on the code base. These imports should be cleaned up from the code if they have no purpose. Clearing these imports will increase the readability of contracts.

Code Location

• LiquidationStrategy.sol#L6
• LiquidationStrategy.sol#L7
• CPMMBaseStrategy.sol#L7

SOLVED: This finding was solved by the GammaSwap team by removing unused imports from the above contracts.

Commit ID: v1-strategies::8e9aa10f9a6ece1c4fb0b8b1b25a1f8e7644bcc0

Solidity contracts can use a special form of comments to provide rich documentation for functions, return variables and more. This special form is named the Ethereum Natural Language Specification Format (NatSpec).

SOLVED: This finding was fixed by the GammaSwap team by adding natspecs to all contracts.

v1-core::5dd28bb25f77bf7b3360b7420507e16688692f60

Open TO-DOs can point to architecture or programming issues that still need to be resolved. Often these kinds of comments indicate areas of complexity or confusion for developers. This provides value and insight to an attacker who aims to cause damage to the protocol.

Code Location

References:

• LiquidationStrategy.sol, #75
• ShortStrategy.sol, #58

SOLVED: This issue was solved by the GammaSwap team by closing open TODOs.

Commit IDs:

• v1-strategies::c1d8b61f3c956aba31eaa23febc50d451da4a7a8
• v1-strategies::6caa88c36e3750e816fbb49168156ae7c1c342ef
• v1-strategies::58dd22e5d56ea43c4cf4dcfbda9a746a58ed0f72