Ajna Protocol - 0xnev's results

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L271

Vulnerability details

Impact

depositTime not updated to 0 in PositionManager.moveLiquidity after owner moves liquidity allows them to call moveLiquidity again to potentially move excess lp positions.

Proof of Concept

PositionManager.sol#L271

function memorializePositions(
    MemorializePositionsParams calldata params_
) external override {
    EnumerableSet.UintSet storage positionIndex = positionIndexes[params_.tokenId];

    IPool   pool  = IPool(poolKey[params_.tokenId]);
    address owner = ownerOf(params_.tokenId);

    uint256 indexesLength = params_.indexes.length;
    uint256 index;

    for (uint256 i = 0; i < indexesLength; ) {
        index = params_.indexes[i];

        // record bucket index at which a position has added liquidity
        // slither-disable-next-line unused-return
        positionIndex.add(index);

        (uint256 lpBalance, uint256 depositTime) = pool.lenderInfo(index, owner);

        Position memory position = positions[params_.tokenId][index];

        // check for previous deposits
        if (position.depositTime != 0) {
            // check that bucket didn't go bankrupt after prior memorialization
            if (_bucketBankruptAfterDeposit(pool, index, position.depositTime)) {
                // if bucket did go bankrupt, zero out the LP tracked by position manager
                position.lps = 0;
            }
        }

        // update token position LP
        position.lps += lpBalance;
        // set token's position deposit time to the original lender's deposit time
        position.depositTime = depositTime;

        // save position in storage
        positions[params_.tokenId][index] = position;

        unchecked { ++i; }
    }

    // update pool LP accounting and transfer ownership of LP to PositionManager contract
    pool.transferLP(owner, address(this), params_.indexes);

    emit MemorializePosition(owner, params_.tokenId, params_.indexes);
}

/**
    *  @inheritdoc IPositionManagerOwnerActions
    *  @dev    === Write state ===
    *  @dev    `poolKey`: update `tokenId => pool` mapping
    *  @dev    === Revert on ===
    *  @dev    provided pool not valid `NotAjnaPool()`
    *  @dev    === Emit events ===
    *  @dev    - `Mint`
    */
function mint(
    MintParams calldata params_
) external override nonReentrant returns (uint256 tokenId_) {
    tokenId_ = _nextId++;

    // revert if the address is not a valid Ajna pool
    if (!_isAjnaPool(params_.pool, params_.poolSubsetHash)) revert NotAjnaPool();

    // record which pool the tokenId was minted in
    poolKey[tokenId_] = params_.pool;

    _mint(params_.recipient, tokenId_);

    emit Mint(params_.recipient, params_.pool, tokenId_);
}

/**
    *  @inheritdoc IPositionManagerOwnerActions
    *  @dev    External calls to `Pool` contract:
    *  @dev    `bucketInfo()`: get from bucket info
    *  @dev    `moveQuoteToken()`: move liquidity between buckets
    *  @dev    === Write state ===
    *  @dev    `positionIndexes`: remove from bucket index
    *  @dev    `positionIndexes`: add to bucket index
    *  @dev    `positions`: update from bucket position
    *  @dev    `positions`: update to bucket position
    *  @dev    === Revert on ===
    *  @dev    - `mayInteract`:
    *  @dev      token id is not a valid / minted id
    *  @dev      sender is not owner `NoAuth()`
    *  @dev      token id not minted for given pool `WrongPool()`
    *  @dev    - positions token to burn has liquidity `LiquidityNotRemoved()`
    *  @dev    === Emit events ===
    *  @dev    - `MoveLiquidity`
    */
function moveLiquidity(
    MoveLiquidityParams calldata params_
) external override mayInteract(params_.pool, params_.tokenId) nonReentrant {
    Position storage fromPosition = positions[params_.tokenId][params_.fromIndex];

    MoveLiquidityLocalVars memory vars;
    vars.depositTime = fromPosition.depositTime;

    // handle the case where owner attempts to move liquidity after they've already done so
    if (vars.depositTime == 0) revert RemovePositionFailed();

    // ensure bucketDeposit accounts for accrued interest
    IPool(params_.pool).updateInterest();

    // retrieve info of bucket from which liquidity is moved  
    (
        vars.bucketLP,
        vars.bucketCollateral,
        vars.bankruptcyTime,
        vars.bucketDeposit,
    ) = IPool(params_.pool).bucketInfo(params_.fromIndex);

    // check that bucket hasn't gone bankrupt since memorialization
    if (vars.depositTime <= vars.bankruptcyTime) revert BucketBankrupt();

    // calculate the max amount of quote tokens that can be moved, given the tracked LP
    vars.maxQuote = _lpToQuoteToken(
        vars.bucketLP,
        vars.bucketCollateral,
        vars.bucketDeposit,
        fromPosition.lps,
        vars.bucketDeposit,
        _priceAt(params_.fromIndex)
    );

    EnumerableSet.UintSet storage positionIndex = positionIndexes[params_.tokenId];

    // remove bucket index from which liquidity is moved from tracked positions
    if (!positionIndex.remove(params_.fromIndex)) revert RemovePositionFailed();

    // update bucket set at which a position has liquidity
    // slither-disable-next-line unused-return
    positionIndex.add(params_.toIndex);

    // move quote tokens in pool
    (
        vars.lpbAmountFrom,
        vars.lpbAmountTo,
    ) = IPool(params_.pool).moveQuoteToken(
        vars.maxQuote,
        params_.fromIndex,
        params_.toIndex,
        params_.expiry
    );

    Position storage toPosition = positions[params_.tokenId][params_.toIndex];

    // update position LP state
    fromPosition.lps -= vars.lpbAmountFrom;
    toPosition.lps   += vars.lpbAmountTo;
    // update position deposit time to the from bucket deposit time
    toPosition.depositTime = vars.depositTime;

    emit MoveLiquidity(
        ownerOf(params_.tokenId),
        params_.tokenId,
        params_.fromIndex,
        params_.toIndex,
        vars.lpbAmountFrom,
        vars.lpbAmountTo
    );
}

if (vars.depositTime == 0) revert RemovePositionFailed();

In PositionManager.moveLiquidity, there is a check to ensure that owner cannot move liquidity for specific position associated with NFT within a bucket after they have already done so. This check is useless as fromPosition.depositTime is never reset to 0 to reflect deposit moved after calling moveLiquidity for lp positions associated with NFT at specific bucket

This means that lender with position associated with their NFT can still move liquidity within the original bucket if lp position is worth lesser than when at deposit time based on amount of quote token moved and current exchange rate. Lenders could still move excess lp position via PositionManager.moveLiquidity since lp tracked by position manager at fromIndex bucket is not deleted yet (i.e. depositTime and lps still exist). This allows lenders to move more liquidity than intended and not follow current exchange rate based on liquidity in buckets.

Tools Used

Manual Analysis

Recommendation

LP tracked by position manager at bucket index should be deleted similar to redeemPositions, if not NFT associated with positions still remain and lenders can move/redeem extra lp tokens.

function moveLiquidity(
    MoveLiquidityParams calldata params_
) external override mayInteract(params_.pool, params_.tokenId) nonReentrant {
    ...

    // update position LP state
    fromPosition.lps -= vars.lpbAmountFrom;
    toPosition.lps   += vars.lpbAmountTo;
    // update position deposit time to the from bucket deposit time
    toPosition.depositTime = vars.depositTime;
++  delete fromPosition  

    emit MoveLiquidity(
        ownerOf(params_.tokenId),
        params_.tokenId,
        params_.fromIndex,
        params_.toIndex,
        vars.lpbAmountFrom,
        vars.lpbAmountTo
    );
}

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L320

Vulnerability details

Impact

Position.moveLiquidity can potentially revert due to underflow

Proof of Concept

PositionManager.sol#L320

function moveLiquidity(
    MoveLiquidityParams calldata params_
) external override mayInteract(params_.pool, params_.tokenId) nonReentrant {
    Position storage fromPosition = positions[params_.tokenId][params_.fromIndex];

    MoveLiquidityLocalVars memory vars;
    vars.depositTime = fromPosition.depositTime;

    // handle the case where owner attempts to move liquidity after they've already done so
    if (vars.depositTime == 0) revert RemovePositionFailed();

    // ensure bucketDeposit accounts for accrued interest
    IPool(params_.pool).updateInterest();

    // retrieve info of bucket from which liquidity is moved  
    (
        vars.bucketLP,
        vars.bucketCollateral,
        vars.bankruptcyTime,
        vars.bucketDeposit,
    ) = IPool(params_.pool).bucketInfo(params_.fromIndex);

    // check that bucket hasn't gone bankrupt since memorialization
    if (vars.depositTime <= vars.bankruptcyTime) revert BucketBankrupt();

    // calculate the max amount of quote tokens that can be moved, given the tracked LP
    vars.maxQuote = _lpToQuoteToken(
        vars.bucketLP,
        vars.bucketCollateral,
        vars.bucketDeposit,
        fromPosition.lps,
        vars.bucketDeposit,
        _priceAt(params_.fromIndex)
    );

    EnumerableSet.UintSet storage positionIndex = positionIndexes[params_.tokenId];

    // remove bucket index from which liquidity is moved from tracked positions
    if (!positionIndex.remove(params_.fromIndex)) revert RemovePositionFailed();

    // update bucket set at which a position has liquidity
    // slither-disable-next-line unused-return
    positionIndex.add(params_.toIndex);

    // move quote tokens in pool
    (
        vars.lpbAmountFrom,
        vars.lpbAmountTo,
    ) = IPool(params_.pool).moveQuoteToken(
        vars.maxQuote,
        params_.fromIndex,
        params_.toIndex,
        params_.expiry
    );

    Position storage toPosition = positions[params_.tokenId][params_.toIndex];

    // update position LP state
    fromPosition.lps -= vars.lpbAmountFrom;
    toPosition.lps   += vars.lpbAmountTo;
    // update position deposit time to the from bucket deposit time
    toPosition.depositTime = vars.depositTime;

    emit MoveLiquidity(
        ownerOf(params_.tokenId),
        params_.tokenId,
        params_.fromIndex,
        params_.toIndex,
        vars.lpbAmountFrom,
        vars.lpbAmountTo
    );
}

If lp position is worth more than when at deposit time based on amount of quote token moved which could likely be the case due to interest accrued or simply from exchange rates favoring quote token, moveLiquidity could revert due to underflow as vars.lpbAmountFrom could be greater than fromPosition.lps (i.e. vars.lpbAmountFrom > fromPosition.lps)

Since call to Position.moveLiquidity() is supposed to move all lp positions attached to associated NFT within a bucket, we can simply delete the fromPosition mapping to remove bucket index at which a position has moved liquidity to prevent potential cases where Position.moveLiquidity() could revert due to underflow in this line:

fromPosition.lps -= vars.lpbAmountFrom;

Tools Used

Manual Analysis

Recommendation

LP tracked by position manager at bucket index should be deleted similar to redeemPositions, if not Position.moveLiquidity can likely underflow

function moveLiquidity(
    MoveLiquidityParams calldata params_
) external override mayInteract(params_.pool, params_.tokenId) nonReentrant {
    ...

    // update position LP state
    fromPosition.lps -= vars.lpbAmountFrom;
    toPosition.lps   += vars.lpbAmountTo;
    // update position deposit time to the from bucket deposit time
    toPosition.depositTime = vars.depositTime;
++  delete fromPosition  

    emit MoveLiquidity(
        ownerOf(params_.tokenId),
        params_.tokenId,
        params_.fromIndex,
        params_.toIndex,
        vars.lpbAmountFrom,
        vars.lpbAmountTo
    );
}

Assessed type

Under/Overflow

Issues Template

Low Risk Template

Non-Critical Template

Refactor Issues Template

[L-01] PostionManager.getPositionInfo may return wrong information for positions not deleted after calling PostionManager.moveLiquidity

PositionManager.sol#L488-L496

function getPositionInfo(
    uint256 tokenId_,
    uint256 index_
) external view override returns (uint256, uint256) {
    return (
        positions[tokenId_][index_].lps,
        positions[tokenId_][index_].depositTime
    );
}

When calling PositionManager.moveLiquidity, the fromPosition mapping is not correctly deleted for the position after moving associated liquidity. This may cause getter function of PositionManager.getPositionInfo() to return wrong values for depositTime for lender with associated NFT.

Recommendation: Since deposits has already been moved, depositTime should be zeroed out to signify no deposits at that bucket for lender

[L-02] Lack of access control for PositionManager.mint

PositionManager.sol#L227-L241

function mint(
    MintParams calldata params_
) external override nonReentrant returns (uint256 tokenId_) {
    tokenId_ = _nextId++;

    // revert if the address is not a valid Ajna pool
    if (!_isAjnaPool(params_.pool, params_.poolSubsetHash)) revert NotAjnaPool();

    // record which pool the tokenId was minted in
    poolKey[tokenId_] = params_.pool;

    _mint(params_.recipient, tokenId_);

    emit Mint(params_.recipient, params_.pool, tokenId_);
}

PositionManager.mint lacks access control and allow anybody to mint ERC721 token even if they do not provide liquidity.

Theoretically, for any existing Ajna pool deployed, anybody with sufficient funds to pay gas for minting can mint maximum amount of NFT and DOS anybody from interacting with the pool with an associated NFT. This prevents lenders from minting NFT for that specific Ajna pool and earning rewards from staking.

Recommendation: Consider implementing access control for minting where minting is only allowed for lenders that have already provided liquidity.

[NC-01] Users may not receive deserved reward when RewardsManager.sol contract has low AJNA balance

RewardsManager.sol#L815

function _transferAjnaRewards(uint256 rewardsEarned_) internal {
    // check that rewards earned isn't greater than remaining balance
    // if remaining balance is greater, set to remaining balance
    uint256 ajnaBalance = IERC20(ajnaToken).balanceOf(address(this));
    if (rewardsEarned_ > ajnaBalance) rewardsEarned_ = ajnaBalance;

    if (rewardsEarned_ != 0) {
        // transfer rewards to sender
        IERC20(ajnaToken).safeTransfer(msg.sender, rewardsEarned_);
    }
}

If there are insufficient AJNA to payout rewards, lenders staking may get lesser rewards than expected.

Protocol may want to consider reverting transfer to allow protocol owners to transfer more AJNA tokens to the RewardsManager.sol so that lenders staking can claim rewards as expected. If not, if this is expected behavior, put a notice or warn lenders of this possible scenario.

function _transferAjnaRewards(uint256 rewardsEarned_) internal {
    // check that rewards earned isn't greater than remaining balance
    // if remaining balance is greater, set to remaining balance
    uint256 ajnaBalance = IERC20(ajnaToken).balanceOf(address(this));
--  if (rewardsEarned_ > ajnaBalance) rewardsEarned_ = ajnaBalance;
++  if (rewardsEarned_ > ajnaBalance) revert InsufficientBalance()

    if (rewardsEarned_ != 0) {
        // transfer rewards to sender
        IERC20(ajnaToken).safeTransfer(msg.sender, rewardsEarned_);
    }
}

[NC-02] Lender can unexpectedly claim rewards from all existing positions in buckets when RewardsManager.movingStakedLiquidity is called instead of just specified buckets where postions is moved

RewardsManager.sol#L153-L159

function moveStakedLiquidity(
    uint256 tokenId_,
    uint256[] memory fromBuckets_,
    uint256[] memory toBuckets_,
    uint256 expiry_
) external nonReentrant override {
    StakeInfo storage stakeInfo = stakes[tokenId_];

    if (msg.sender != stakeInfo.owner) revert NotOwnerOfDeposit();

    // check move array sizes match to be able to match on index
    uint256 fromBucketLength = fromBuckets_.length;
    if (fromBucketLength != toBuckets_.length) revert MoveStakedLiquidityInvalid();

    address ajnaPool = stakeInfo.ajnaPool;
    uint256 curBurnEpoch = IPool(ajnaPool).currentBurnEpoch();

    // claim rewards before moving liquidity, if any
    _claimRewards(
        stakeInfo,
        tokenId_,
        curBurnEpoch,
        false,
        ajnaPool
    );
    ...

Lenders taking NFT with associated positions can unexpectedly claim rewards all rewards from existing buckets when RewardsManager.movingStakedLiquidity is called instead of just specified postions where bucket is moved, since the function calls _claimRewards which claims rewards for every single position attached to NFT in every single bucket index where position is in.

Recommendation: Consider only claiming rewards in buckets from where positions are moved.

[NC-03] User can still memorialize and stake positions to NFT at buckets after they have already move position

PositionManager.sol#L170 The PositionManager.moveLiquidity() expects lenders to only move liquidity associated with NFT in specific buckets once. However, given buckets where position are moved from are not deleted as noted in [L-01], lenders can still call PositionManager.memorializePositions() to attach positions to NFT and subsequently stake these positions. Although they potentially do not earn any rewards or little rewards from this, it is still best to prevent this from happening by deleting fromBuckets in PositionManager.moveLiquidity()to reflect consistent accounting.

[NC-04] Consider using block.timestamp instead of block.number

GrantFund.sol Funding.sol ExtraordinaryFunding.sol StandardFunding.sol Average block time might change in the future and may cause inconsistencies in hard coded 12-second block periods. Hence, consider using block.timestamp instead of block.number for the funding contracts (GrantFund.sol, Funding.sol, ExtraordinaryFunding.sol, StandardFunding.sol)

In addition, funding contracts could be deployed in multiple different networks instead of just ethereum mainnet and potentially save gas in the grant coordination process.

[NC-05] <x> += <y> costs more gas than <x> = <x> + <y> for state variables

Using the addition operator instead of plus-equals saves 113 gas

StandardFunding.sol#L157 StandardFunding.sol#L217

/StandardFunding.sol
157:        treasury -= gbc;

217:        treasury += (fundsAvailable - totalTokensRequested);

ExtraordinaryFunding.sol#L78

78:        treasury -= tokensRequested;

[NC-06] Use custom error revert instead of require

PositionManager.sol#L520

520:        require(_exists(tokenId_));

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

[NC-07] Multiple accesses of a mapping/array should use a local variable cache

poolKey[tokenId_] in PositionManager.tokenURI()

PositionManager.sol#L522-L523 PositionManager.sol#L529

522:        address collateralTokenAddress = IPool(poolKey[tokenId_]).collateralAddress();
523:        address quoteTokenAddress      = IPool(poolKey[tokenId_]).quoteTokenAddress();
529:            pool:                  poolKey[tokenId_]

positions[tokenId_][index_] in getPositionInfo

PositionManager.sol#L493-L494

493:            positions[tokenId_][index_].lps,
494:            positions[tokenId_][index_].depositTime

The instances above 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.

[R-01] Confusing mapping name

PositionManager.sol#L59

mapping(uint256 => EnumerableSet.UintSet)        internal positionIndexes;

Consider renaming the mapping positionIndexes in PositionManager.sol to positionBucketIndexes

[R-02] Repeated owner checks can be refactored into a modifer

RewardsManager.sol#L120 RewardsManager.sol#L143 RewardsManager.sol#L275

if (msg.sender != stakeInfo.owner) revert NotOwnerOfDeposit();`

Consider refactoring repeated NFT owner only check to a modifer in RewardsManager.sol

[R-03] Use WAD constant declared

Maths.sol

6:     uint256 public constant WAD = 10**18;

WAD constant is already declared in Maths.sol so consider using it to improve readability in all arithmetic functions involving WAD inputs in Maths.sol

[R-04] Do not need to use += operator, = can be used instead

RewardsManager.sol#L801

rewards_ += Maths.wmul(UPDATE_CLAIM_REWARD, Maths.wmul(burnFactor, interestFactor));

For the calculation of rewards_ in RewardsManager. _updateBucketExchangeRateAndCalculateRewards(), = can be used instead of += for return reward variables since the variable is always initialized as 0 for every external call given it only updates the exchange rate of a specific bucket.

[R-05] else if block in Maths.wsqrt() can be removed

Maths.sol#L26-L28

function wsqrt(uint256 y) internal pure returns (uint256 z) {
    if (y > 3) {
        z = y;
        uint256 x = y / 2 + 1;
        while (x < z) {
            z = x;
            x = (y / x + x) / 2;
        }
    } else if (y != 0) {
        z = 1;
    }
    // convert z to a WAD
    z = z * 10**9;
}

Since the math library takes in WAD as a number, that is y has a precision of 18 decimals, the logic in the else if block will almost never be executed and hence can be removed to save deployment cost. Furthermore, Maths.wsqrt() is only used in StandardFunding.sol.getFundingPowerVotes(), which takes in votingPower_ in WAD to calculate discrete votes in WAD.

[R-06] RewardsManager._getBurnEpochsClaimed() can be refactored

RewardsManager.sol#L610-L612

function _getBurnEpochsClaimed(
    uint256 lastClaimedEpoch_,
    uint256 burnEpochToStartClaim_
) internal pure returns (uint256[] memory burnEpochsClaimed_) {
    uint256 numEpochsClaimed = burnEpochToStartClaim_ - lastClaimedEpoch_;

    burnEpochsClaimed_ = new uint256[](numEpochsClaimed);

    uint256 i;
    uint256 claimEpoch = lastClaimedEpoch_ + 1;
    while (claimEpoch <= burnEpochToStartClaim_) {
        burnEpochsClaimed_[i] = claimEpoch;

        // iterations are bounded by array length (which is itself bounded), preventing overflow / underflow
        unchecked {
            ++i;
            ++claimEpoch;
        }
    }
}

Can reduce SLOC and potentially save some gas due to reducing 1 MSTORE and 1 MLOAD

function _getBurnEpochsClaimed(
    uint256 lastClaimedEpoch_,
    uint256 burnEpochToStartClaim_
) internal pure returns (uint256[] memory burnEpochsClaimed_) {
--  uint256 numEpochsClaimed = burnEpochToStartClaim_ - lastClaimedEpoch_;    
++  burnEpochsClaimed_ = new uint256[](burnEpochToStartClaim_ - lastClaimedEpoch_);

--  burnEpochsClaimed_ = new uint256[](numEpochsClaimed);    

    uint256 i;
    uint256 claimEpoch = lastClaimedEpoch_ + 1;
    while (claimEpoch <= burnEpochToStartClaim_) {
        burnEpochsClaimed_[i] = claimEpoch;

        // iterations are bounded by array length (which is itself bounded), preventing overflow / underflow
        unchecked {
            ++i;
            ++claimEpoch;
        }
    }
}

[R-07] Use constant instead of immutable for ajnaTokenAddress in Funding.sol

Funding.sol#L21

address public immutable ajnaTokenAddress = 0x9a96ec9B57Fb64FbC60B423d1f4da7691Bd35079;

Since there is no constructors in all the funding contracts inheriting Funding.sol, ajnaTokenAddress is only set once at contract deployment and can be set as constant.

address public constant AJNA_TOKEN_ADDRESS = 0x9a96ec9B57Fb64FbC60B423d1f4da7691Bd35079;

[R-08] Implement StandardFunding._setNewDistributionId() in StandardFunding.startNewDistributionPeriod() directly

StandardFunding.sol#L227-L229 StandardFunding.sol#L146

-- newDistributionId_ = _setNewDistributionId();
++ newDistributionId_ = _currentDistributionId += 1

Consider implementing the private function StandardFunding._setNewDistributionId() in StandardFunding.startNewDistributionPeriod() directly since it is only use once in that function.

Could potentially save deployment cost as the function StandardFunding._setNewDistributionId() is removed

All gas savings benchmark are retrieved from comparisons from unit tests

[G-01] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

PositionManager.sol#L62

62:    uint176 private _nextId = 1;

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Each operation involving a uint8 costs extra as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed

Saves ~16255 gas on deployment and on average 256 gas for PositionManager.mint() function that uses the _nextId variable.

PositionManager.sol Deployment Cost:

PositionManager.mint() gas savings:

[G-02] Do not need to use += operator, = can be used instead

RewardsManager.sol#L801

rewards_ += Maths.wmul(UPDATE_CLAIM_REWARD, Maths.wmul(burnFactor, interestFactor));

For the calculation of rewards_ in RewardsManager. _updateBucketExchangeRateAndCalculateRewards(), = can be used instead of += for return reward variables since the variable is always initialized as 0 for every external call given it only updates the exchange rate of a specific bucket.

The RewardsManager. _updateBucketExchangeRateAndCalculateRewards() is called by RewardsManager._updateBucketExchangeRates() which is in turn called by moveStakedLiquidity(), stake(), updateBucketExchangeRatesAndClaim() and _claimRewards().

_claimRewards is inturn called by unstake(), moveStakedLiquidity() and claimRewards()

Saves ~2000 gas on deployment and ~60631 gas throughout all the functions mentioned above.

RewardsManager.sol Deployment Cost:

Functions gas savings: Before

After

[G-03] else if block in Maths.wsqrt() can be removed

Maths.sol#L26-L28

function wsqrt(uint256 y) internal pure returns (uint256 z) {
    if (y > 3) {
        z = y;
        uint256 x = y / 2 + 1;
        while (x < z) {
            z = x;
            x = (y / x + x) / 2;
        }
    } else if (y != 0) {
        z = 1;
    }
    // convert z to a WAD
    z = z * 10**9;
}

Since the math library takes in WAD as a number, that is y has a precision of 18 decimals, the logic in the else if block will almost never be executed and hence can be removed to save deployment cost. Furthermore, Maths.wsqrt() is only used in StandardFunding.sol.getFundingPowerVotes(), which takes in votingPower_ in WAD to calculate discrete votes in WAD.

Based on testSqrt() in Maths.t.sol, and using `saves ~ 11 gas per call

Note: assertEq(Maths.wsqrt(2), 1 * 1e9); was removed from Maths.t.sol to test gas savings.

Before
[PASS] testSqrt() (gas: 38937)

After
[PASS] testSqrt() (gas: 38893)

[G-04] Implement StandardFunding._setNewDistributionId() in StandardFunding.startNewDistributionPeriod() directly

StandardFunding.sol#L227-L229 StandardFunding.sol#L146

-- newDistributionId_ = _setNewDistributionId();
++ newDistributionId_ = _currentDistributionId += 1

Consider implementing the private function StandardFunding._setNewDistributionId() in StandardFunding.startNewDistributionPeriod() directly since it is only use once in that function.

Could potentially save gas as the function StandardFunding._setNewDistributionId() is removed

Saves ~3200 gas for GrantFund.sol deployment cost and on average ~22 gas for call to StandardFunding.startNewDistributionPeriod()

GrantFund.sol Deployment Cost:

StandardFunding._setNewDistributionId() gas savings

[G-05] Use constants instead of immutable for ajnaTokenAddress in Funding.sol

Funding.sol#L21

address public immutable ajnaTokenAddress = 0x9a96ec9B57Fb64FbC60B423d1f4da7691Bd35079;

Since there is no constructors in all the funding contracts inheriting Funding.sol, ajnaTokenAddress is only set once at contract deployment and can be set as constant.

address public constant AJNA_TOKEN_ADDRESS = 0x9a96ec9B57Fb64FbC60B423d1f4da7691Bd35079;

Funding.sol is inherited by ExtraordinaryFunding.sol and StandardFunding.sol which are in turn inherited by GrantFund.sol. As such, saves ~16919 gas for GrantFund.sol deployment cost.

GrantFund.sol Deployment Cost:

[G-06] <x> += <y> costs more gas than <x> = <x> + <y> for state variables

Using the addition operator instead of plus-equals saves 113 gas

There are 3 instances saves: 339 gas

StandardFunding.sol#L157 StandardFunding.sol#L217

157:        treasury -= gbc;

217:        treasury += (fundsAvailable - totalTokensRequested);

ExtraordinaryFunding.sol#L78

78:        treasury -= tokensRequested;

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

There are 5 instances saves: 210 gas

poolKey[tokenId_] in PositionManager.tokenURI()

PositionManager.sol#L522-L523 PositionManager.sol#L529

522:        address collateralTokenAddress = IPool(poolKey[tokenId_]).collateralAddress();
523:        address quoteTokenAddress      = IPool(poolKey[tokenId_]).quoteTokenAddress();
529:            pool:                  poolKey[tokenId_]

positions[tokenId_][index_] in getPositionInfo

PositionManager.sol#L493-L494

493:            positions[tokenId_][index_].lps,
494:            positions[tokenId_][index_].depositTime

The instances above 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.