QuickSwap and StellaSwap contest - __141345__'s results

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/libraries/AdaptiveFee.sol#L70-L108

Vulnerability details

Impact

The evaluation of exp function will be inaccurate, and further affect the accuracy of sigmoid() function, eventually affect fee calculation. Also the calculation takes quite a lot of gas to calculate to 8th term.

Proof of Concept

x/g takes value between 0 to 6, but taylor expansion maintains good approximation only near 0. When x/g is close to 5, the error is around 7% for exp() and 7% forsigmoid() respectively. When x/g is close to 6, the error could go up to 15% for exp() and 18% for sigmoid().

// src/core/contracts/libraries/AdaptiveFee.sol
  function exp(
    uint256 x,
    uint16 g,
    uint256 gHighestDegree
  ) internal pure returns (uint256 res) {
    // calculating:
    // g**8 + x * g**7 + (x**2 * g**6) / 2 + (x**3 * g**5) / 6 + (x**4 * g**4) / 24 + (x**5 * g**3) / 120 + (x**6 * g^2) / 720 + x**7 * g / 5040 + x**8 / 40320

    // x**8 < 152 bits (19*8) and g**8 < 128 bits (8*16)
    // so each summand < 152 bits and res < 155 bits
    uint256 xLowestDegree = x;
    res = gHighestDegree; // g**8

    gHighestDegree /= g; // g**7
    res += xLowestDegree * gHighestDegree;

    gHighestDegree /= g; // g**6
    xLowestDegree *= x; // x**2
    res += (xLowestDegree * gHighestDegree) / 2;

    gHighestDegree /= g; // g**5
    xLowestDegree *= x; // x**3
    res += (xLowestDegree * gHighestDegree) / 6;

    gHighestDegree /= g; // g**4
    xLowestDegree *= x; // x**4
    res += (xLowestDegree * gHighestDegree) / 24;

    gHighestDegree /= g; // g**3
    xLowestDegree *= x; // x**5
    res += (xLowestDegree * gHighestDegree) / 120;

    gHighestDegree /= g; // g**2
    xLowestDegree *= x; // x**6
    res += (xLowestDegree * gHighestDegree) / 720;

    xLowestDegree *= x; // x**7
    res += (xLowestDegree * g) / 5040 + (xLowestDegree * x) / (40320);
  }

Tools Used

Manual analysis.

Recommended Mitigation Steps

The value of exp(1), exp(2), exp(3), exp(4), exp(5), exp(6) can be pre-calculated and saved to constants, then be used as denominator or multiplier.

For example, to calculate exp(3.48), what we can do is calculate exp(0.48), and then multiply by exp(3).

When the power is less than 0.5, taylor expansion up to x^3 can give good accuracy. exp(0.5) and corresponding sigmoid() has maximum error in the order of 2e-4 to 1e-4.

Missing Time locks

When critical parameters of systems need to be changed, it is required to broadcast the change via event emission and recommended to enforce the changes after a time-delay. This is to allow system users to be aware of such critical changes and give them an opportunity to exit or adjust their engagement with the system accordingly. None of the onlyOwner functions that change critical protocol addresses/parameters have a timelock for a time-delayed change to alert: (1) users and give them a chance to engage/exit protocol if they are not agreeable to the changes (2) team in case of compromised owner(s) and give them a chance to perform incident response.

Instances number of this issue:

// src/core/contracts/AlgebraFactory.sol
  function setFarmingAddress(address _farmingAddress) external override onlyOwner {}

  function setVaultAddress(address _vaultAddress) external override onlyOwner {}

  function setBaseFeeConfiguration() external override onlyOwner {}

// src/core/contracts/AlgebraPool.sol
  function setCommunityFee(uint8 communityFee0, uint8 communityFee1) external override lock onlyFactoryOwner {}

  function setIncentive(address virtualPoolAddress) external override {}

  function setLiquidityCooldown(uint32 newLiquidityCooldown) external override onlyFactoryOwner {}

Suggestion: Users may be surprised when critical parameters are changed. Furthermore, it can erode users' trust since they can’t be sure the protocol rules won’t be changed later on. Compromised owner keys may be used to change protocol addresses/parameters to benefit attackers. Without a time-delay, authorized owners have no time for any planned incident response. (May be medium since owner can change critical parameters at anytime that can affect the users poorly).

Missing Zero-address Validation

Description: Lack of zero-address validation on address parameters may lead to transaction reverts, waste gas, require resubmission of transactions and may even force contract redeployments in certain cases within the protocol.

Suggestion: Consider adding explicit zero-address validation on input parameters of address type.

Instances number of this issue:

// src/core/contracts/AlgebraFactory.sol
  function setFarmingAddress(address _farmingAddress) external override onlyOwner {
    require(farmingAddress != _farmingAddress);
    emit FarmingAddress(_farmingAddress);
    farmingAddress = _farmingAddress;
  }

  function setVaultAddress(address _vaultAddress) external override onlyOwner {
    require(vaultAddress != _vaultAddress);
    emit VaultAddress(_vaultAddress);
    vaultAddress = _vaultAddress;
  }

// src/core/contracts/AlgebraPool.sol
  function setIncentive(address virtualPoolAddress) external override {
    require(msg.sender == IAlgebraFactory(factory).farmingAddress());
    activeIncentive = virtualPoolAddress;

    emit Incentive(virtualPoolAddress);
  }

Code Structure Deviates From Best-Practice

Description: The best-practice layout for a contract should follow the following order: state variables, events, modifiers, constructor and functions. Function ordering helps readers identify which functions they can call and find constructor and fallback functions easier. Functions should be grouped according to their visibility and ordered as: constructor, receive function (if exists), fallback function (if exists), external, public, internal, private. Some constructs deviate from this recommended best-practice: Modifiers are in the middle of contracts. External/public functions are mixed with internal/private ones. Few events are declared in contracts while most others are in corresponding interfaces.

Suggestion: Consider adopting recommended best-practice for code structure and layout.

src/core/contracts/AlgebraFactory.sol

Missing or Incomplete NatSpec

Description: Some functions are missing @notice/@dev NatSpec comments for the function, @param for all/some of their parameters and @return for return values. Given that NatSpec is an important part of code documentation, this affects code comprehension, auditability and usability.

Suggestion: Consider adding in full NatSpec comments for all functions to have complete code documentation for future use.

src/core/contracts/AlgebraFactory.sol src/core/contracts/AlgebraPool.sol

Not using the named return variables in the function is confusing

Some functions have return statement even with named return variables. Consider changing the variable to be an unnamed one.

Instances number of this issue: 20

// src/core/contracts/libraries/AdaptiveFee.sol
  function getFee() internal pure returns (uint16 fee) {}

  function sigmoid() internal pure returns (uint256 res) {}

// src/core/contracts/AlgebraPool.sol
  function getInnerCumulatives()
    returns (
      int56 innerTickCumulative,
      uint160 innerSecondsSpentPerLiquidity,
      uint32 innerSecondsSpent
    ) {}

  function getTimepoints()
    external
    view
    override
    returns (
      int56[] memory tickCumulatives,
      uint160[] memory secondsPerLiquidityCumulatives,
      uint112[] memory volatilityCumulatives,
      uint256[] memory volumePerAvgLiquiditys
    ) {}

  function _writeTimepoint() private returns (uint16 newTimepointIndex) {}

  function _getSingleTimepoint(
    uint32 blockTimestamp,
    uint32 secondsAgo,
    int24 startTick,
    uint16 timepointIndex,
    uint128 liquidityStart
  )
    private
    view
    returns (
      int56 tickCumulative,
      uint160 secondsPerLiquidityCumulative,
      uint112 volatilityCumulative,
      uint256 volumePerAvgLiquidity
    ) {}

// src/core/contracts/DataStorageOperator.sol
  function getTimepoints()
    external
    view
    override
    onlyPool
    returns (
      int56[] memory tickCumulatives,
      uint160[] memory secondsPerLiquidityCumulatives,
      uint112[] memory volatilityCumulatives,
      uint256[] memory volumePerAvgLiquiditys
    ) {}

  function getAverages() external view override onlyPool returns (uint112 TWVolatilityAverage, uint256 TWVolumePerLiqAverage) {}

  function write() external override onlyPool returns (uint16 indexUpdated) {}

  function calculateVolumePerLiquidity() external pure override returns (uint128 volumePerLiquidity) {}

  function getFee() external view override onlyPool returns (uint16 fee) {}

// src/core/contracts/libraries/DataStorage.sol
  function binarySearch() private view returns (Timepoint storage beforeOrAt, Timepoint storage atOrAfter) {}

  function getSingleTimepoint() internal view returns (Timepoint memory targetTimepoint) {}

  function getAverages() internal view returns (uint88 volatilityAverage, uint256 volumePerLiqAverage) {}

// 
  function getNewPriceAfterInput() internal pure returns (uint160 resultPrice) {}

  function getNewPriceAfterOutput() internal pure returns (uint160 resultPrice) {}

  function getNewPrice() internal pure returns (uint160 resultPrice) {}

// src/core/contracts/libraries/TickManager.sol
  function cross() internal returns (int128 liquidityDelta) {}

// src/core/contracts/libraries/TickTable.sol
  function getMostSignificantBit() internal pure returns (uint8 mostBitPos) {}

  function nextTickInTheSameRow() internal view returns (int24 nextTick, bool initialized) {}

No need to use additional variable

Some function can directly return the results, no need to allocate for new variables.

Instances number of this issue: 4

// src/core/contracts/libraries/TokenDeltaMath.sol
  function getToken0Delta() internal pure returns (uint256 token0Delta) {
    // ...
    token0Delta = roundUp
      ? FullMath.divRoundingUp(FullMath.mulDivRoundingUp(priceDelta, liquidityShifted, priceUpper), priceLower)
      : FullMath.mulDiv(priceDelta, liquidityShifted, priceUpper) / priceLower;
  }

  function getToken1Delta() internal pure returns (uint256 token1Delta) {
    // ...
    token1Delta = roundUp ? FullMath.mulDivRoundingUp(priceDelta, liquidity, Constants.Q96) : FullMath.mulDiv(priceDelta, liquidity, Constants.Q96);
  }

  function getToken0Delta(
    uint160 priceLower,
    uint160 priceUpper,
    int128 liquidity
  ) internal pure returns (int256 token0Delta) {
    token0Delta = liquidity >= 0
      ? getToken0Delta(priceLower, priceUpper, uint128(liquidity), true).toInt256()
      : -getToken0Delta(priceLower, priceUpper, uint128(-liquidity), false).toInt256();
  }


  function getToken1Delta(
    uint160 priceLower,
    uint160 priceUpper,
    int128 liquidity
  ) internal pure returns (int256 token1Delta) {
    token1Delta = liquidity >= 0
      ? getToken1Delta(priceLower, priceUpper, uint128(liquidity), true).toInt256()
      : -getToken1Delta(priceLower, priceUpper, uint128(-liquidity), false).toInt256();
  }

for-loop array.length should not be looked up in every loop

even for memory variables. The demo of the loop gas comparison can be seen here.

Instances number of this issue: 1

// src/core/contracts/libraries/DataStorage.sol
    for (uint256 i = 0; i < secondsAgos.length; i++) {}

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Instances number of this issue: 1

// src/core/contracts/libraries/DataStorage.sol
    for (uint256 i = 0; i < secondsAgos.length; i++) {}

The demo of the loop gas comparison can be seen here.

++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops

This saves 30-40 gas per loop

Instances number of this issue: 1

// src/core/contracts/libraries/DataStorage.sol
    for (uint256 i = 0; i < secondsAgos.length; i++) {}

Variables referred multiple times can be cached in local memory

Each storage read uses opcode sload which costs 100 gas (warm access), while memory read uses mload which only cost 3 gas. (reference) Even for a memory struct variable, the member variable access still has overhead. It can be saved further by caching the final result.

Instances number of this issue: 1 secondsAgos.length can be cached. contracts/VotingEscrow.sol

// src/core/contracts/libraries/DataStorage.sol
294-307:
    tickCumulatives = new int56[](secondsAgos.length);
    secondsPerLiquidityCumulatives = new uint160[](secondsAgos.length);
    volatilityCumulatives = new uint112[](secondsAgos.length);
    volumePerAvgLiquiditys = new uint256[](secondsAgos.length);
    // ...
    for (uint256 i = 0; i < secondsAgos.length; i++) {

Splitting require() statements that use &&

REQUIRE() statements with multiple conditions can be split.

See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper.

The demo of the gas comparison can be seen here.

Instances number of this issue: 6

// src/core/contracts/AlgebraFactory.sol
110:    require(gamma1 != 0 && gamma2 != 0 && volumeGamma != 0, 'Gammas must be > 0');


// src/core/contracts/AlgebraPool.sol
739:    require(limitSqrtPrice < currentPrice && limitSqrtPrice > TickMath.MIN_SQRT_RATIO, 'SPL');
743:    require(limitSqrtPrice > currentPrice && limitSqrtPrice < TickMath.MAX_SQRT_RATIO, 'SPL');
953:    require((communityFee0 <= Constants.MAX_COMMUNITY_FEE) && (communityFee1 <= Constants.MAX_COMMUNITY_FEE));
968:    require(newLiquidityCooldown <= Constants.MAX_LIQUIDITY_COOLDOWN && liquidityCooldown != newLiquidityCooldown);

// src/core/contracts/DataStorageOperator.sol
46:     require(_feeConfig.gamma1 != 0 && _feeConfig.gamma2 != 0 && _feeConfig.volumeGamma != 0, 'Gammas must be > 0');

Use a more recent version of solidity

Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

Use custom errors rather than require() strings

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

Instances number of this issue: 59

// src/core/contracts/AlgebraFactory.sol
43:     require(msg.sender == owner);
60:     require(tokenA != tokenB);
62:     require(token0 != address(0));
63:     require(poolByPair[token0][token1] == address(0));
78:     require(owner != _owner);
85:     require(farmingAddress != _farmingAddress);
92:     require(vaultAddress != _vaultAddress);
109:    require(uint256(alpha1) + uint256(alpha2) + uint256(baseFee) <= type(uint16).max, 'Max fee exceeded');
110:    require(gamma1 != 0 && gamma2 != 0 && volumeGamma != 0, 'Gammas must be > 0');

// src/core/contracts/AlgebraPool.sol
55:     require(msg.sender == IAlgebraFactory(factory).owner());
60:     require(topTick < TickMath.MAX_TICK + 1, 'TUM');
61:     require(topTick > bottomTick, 'TLU');
62:     require(bottomTick > TickMath.MIN_TICK - 1, 'TLM');
122:     require(_lower.initialized);
134:     require(_upper.initialized);
194:     require(globalState.price == 0, 'AI');
224:     require(currentLiquidity > 0, 'NP'); // Do not recalculate the empty ranges
229:     require((_blockTimestamp() - lastLiquidityAddTimestamp) >= _liquidityCooldown);
434:    require(liquidityDesired > 0, 'IL');
454:    if (amount0 > 0) require((receivedAmount0 = balanceToken0() - receivedAmount0) > 0, 'IIAM');
455:    if (amount1 > 0) require((receivedAmount1 = balanceToken1() - receivedAmount1) > 0, 'IIAM');
469:    require(liquidityActual > 0, 'IIL2');
474:    require((amount0 = uint256(amount0Int)) <= receivedAmount0, 'IIAM2');
475:    require((amount1 = uint256(amount1Int)) <= receivedAmount1, 'IIAM2');
608:    require(balance0Before.add(uint256(amount0)) <= balanceToken0(), 'IIA');
614:    require(balance1Before.add(uint256(amount1)) <= balanceToken1(), 'IIA');
636:    require(globalState.unlocked, 'LOK');
641:    require((amountRequired = int256(balanceToken0().sub(balance0Before))) > 0, 'IIA');
645:    require((amountRequired = int256(balanceToken1().sub(balance1Before))) > 0, 'IIA');
731:    require(unlocked, 'LOK');
733:    require(amountRequired != 0, 'AS');
739:    require(limitSqrtPrice < currentPrice && limitSqrtPrice > TickMath.MIN_SQRT_RATIO, 'SPL');
743:    require(limitSqrtPrice > currentPrice && limitSqrtPrice < TickMath.MAX_SQRT_RATIO, 'SPL');
898:    require(_liquidity > 0, 'L');
921:    require(balance0Before.add(fee0) <= paid0, 'F0');
935:    require(balance1Before.add(fee1) <= paid1, 'F1');
953:    require((communityFee0 <= Constants.MAX_COMMUNITY_FEE) && (communityFee1 <= Constants.MAX_COMMUNITY_FEE));
960:    require(msg.sender == IAlgebraFactory(factory).farmingAddress());
968:    require(newLiquidityCooldown <= Constants.MAX_LIQUIDITY_COOLDOWN && liquidityCooldown != newLiquidityCooldown);

// src/core/contracts/AlgebraPoolDeployer.sol
22:     require(msg.sender == factory);
27:     require(msg.sender == owner);
37:     require(_factory != address(0));
38:     require(factory == address(0));

// src/core/contracts/DataStorageOperator.sol
27:     require(msg.sender == pool, 'only pool can call this');
43:     require(msg.sender == factory || msg.sender == IAlgebraFactory(factory).owner());
45:     require(uint256(_feeConfig.alpha1) + uint256(_feeConfig.alpha2) + uint256(_feeConfig.baseFee) <= type(uint16).max, 'Max fee exceeded');
46:     require(_feeConfig.gamma1 != 0 && _feeConfig.gamma2 != 0 && _feeConfig.volumeGamma != 0, 'Gammas must be > 0');

// src/core/contracts/base/PoolState.sol
41:     require(globalState.unlocked, 'LOK');

// src/core/contracts/libraries/DataStorage.sol
238:    require(lteConsideringOverflow(self[oldestIndex].blockTimestamp, target, time), 'OLD');
369:    require(!self[0].initialized);

// src/core/contracts/libraries/PriceMovementMath.sol
52:     require(price > 0);
53:     require(liquidity > 0);
70:     require((product = amount * price) / amount == price); 
71:     require(liquidityShifted > product); 
87:     require(price > quotient);

// src/core/contracts/libraries/TickManager.sol
96:     require(liquidityTotalAfter < Constants.MAX_LIQUIDITY_PER_TICK + 1, 'LO');

// src/core/contracts/libraries/TickTable.sol
15:     require(tick % Constants.TICK_SPACING == 0, 'tick is not spaced');

// src/core/contracts/libraries/TokenDeltaMath.sol
30:     require(priceDelta < priceUpper); // forbids underflow and 0 priceLower
51:     require(priceUpper >= priceLower);

Functions guaranteed to revert when called by normal users can be marked payable

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.

Instances number of this issue: 14

// src/core/contracts/AlgebraFactory.sol
77:   function setOwner(address _owner) external override onlyOwner {
84:   function setFarmingAddress(address _farmingAddress) external override onlyOwner {
91:   function setVaultAddress(address _vaultAddress) external override onlyOwner {
98:   function setBaseFeeConfiguration() external override onlyOwner {

// src/core/contracts/AlgebraPoolDeployer.sol
36:   function setFactory(address _factory) external override onlyOwner {
44-49:
      function deploy() external override onlyFactory returns (address pool) {


src/core/contracts/AlgebraPool.sol
952:  function setCommunityFee(uint8 communityFee0, uint8 communityFee1) external override lock onlyFactoryOwner {
967:  function setLiquidityCooldown(uint32 newLiquidityCooldown) external override onlyFactoryOwner {

// src/core/contracts/DataStorageOperator.sol
37:  function initialize(uint32 time, int24 tick) external override onlyPool {
53-69
    function getSingleTimepoint()
      external
      view
      override
      onlyPool
      returns ()
88-104
    function getTimepoints()
      external
      view
      override
      onlyPool
      returns ()
110-115
    function getAverages() external view override onlyPool returns (uint112 TWVolatilityAverage, uint256 TWVolumePerLiqAverage) {
120-126
    function write() external override onlyPool returns (uint16 indexUpdated) {
150-150
    function getFee() external view override onlyPool returns (uint16 fee) {