Ajna Protocol - Aymen0909's results

QA Report

Summary

Findings

1- Many functions not reverting when they are supposed to in PositionManager :

Risk : Low

The comments of each function indicates the conditions for which the function should revert but there are many functions in the PositionManager contract that do not revert when they are supposed to, If this is an error in the comments then they should be rewitten to correspand to the code but if the functions are really supposed to revert as specified in the comments then the functions code should be reviewed and required checks must be added.

Proof of Concept

Instances include :

File: PositionManager.sol Line 157-216

In the code linked above the function memorializePositions is supposed to revert when the positions token to burn has liquidity but it does not.

File: PositionManager.sol Line 243-333

In the code linked above the function moveLiquidity is supposed to revert when the positions token to burn has liquidity but it does not.

Mitigation

Review the code of the aferomentioned functions and add the required checks if necessary

2- arrays length not checked in the _execute function :

Risk : Low

The _execute function is used to excute the calldata of the passed proposals, for the function to work correctely all the array passed as arguments must have the same length and if there is a mismatch in the number of items of one of the arrays the proposals may not be fully executed or excuted in a wrong order.

Proof of Concept

Instances include:

File: Funding.sol Line 52-66

function _execute(
    uint256 proposalId_,
    address[] memory targets_,
    uint256[] memory values_,
    bytes[] memory calldatas_
) internal {
    // use common event name to maintain consistency with tally
    emit ProposalExecuted(proposalId_);

    string memory errorMessage = "Governor: call reverted without message";
    for (uint256 i = 0; i < targets_.length; ++i) {
        (bool success, bytes memory returndata) = targets_[i].call{value: values_[i]}(calldatas_[i]);
        Address.verifyCallResult(success, returndata, errorMessage);
    }
}

Mitigation

Add arrays length checks in the _execute function.

3- Immutable state variables lack zero address checks :

Risk : Low

Constructors should check the values written in an immutable state variables(address) is not the address(0).

Proof of Concept

Instances include:

File: PositionManager.sol Line 120-121

erc20PoolFactory  = erc20Factory_;
erc721PoolFactory = erc721Factory_;

File: RewardsManager.sol Line 99

positionManager = positionManager_;

Mitigation

Add non-zero address checks in the constructors for the instances aforementioned.

4- constant should be used instead of immutable :

Risk : Non critical

The type constant should be used for variables that have their values set immediately in the contract code and the immutable type should be used for variables declared in the constructor.

Proof of Concept

There is 1 instance of this issue :

File: Funding.sol Line 21

address public immutable ajnaTokenAddress = 0x9a96ec9B57Fb64FbC60B423d1f4da7691Bd35079;

Mitigation

Use constant instead of immutable in the instances aforementioned.

Gas Optimizations

Summary

Findings

1- Using storage instead of memory for struct/array saves gas :

When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read.

The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct.

There are 8 instances of this issue :

File: RewardsManager.sol Line 442

BucketState memory bucketSnapshot = stakes[tokenId_].snapshot[bucketIndex];

File: StandardFunding.sol Line 203

uint256[] memory fundingProposalIds = _fundedProposalSlates[fundedSlateHash];

File: StandardFunding.sol Line 209

Proposal memory proposal = _standardFundingProposals[fundingProposalIds[i]];

File: StandardFunding.sol Line 379

QuarterlyDistribution memory currentDistribution = _distributions[_currentDistributionId];

File: StandardFunding.sol Line 435

Proposal memory proposal = _standardFundingProposals[proposalIds_[i]];

File: StandardFunding.sol Line 575

QuarterlyDistribution memory currentDistribution = _distributions[_currentDistributionId];

File: StandardFunding.sol Line 1010

QuarterlyDistribution memory currentDistribution = _distributions[distributionId_];

File: StandardFunding.sol Line 1011

QuadraticVoter        memory voter               = _quadraticVoters[currentDistribution.id][account_];

2- storage variable should be cached into memory variables instead of re-reading them :

The instances below point to the second+ access of a state variable within a function, the caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read, thus saves 100gas for each instance.

There is 1 instance of this issue :

File: StandardFunding.sol Line 641

if (support == 0 && existingVote.votesUsed > 0 || support == 1 && existingVote.votesUsed < 0)

In the code linked above the value of existingVote.votesUsed is read multiple times (2) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

3- Use unchecked blocks to save gas :

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

There are 3 instances of this issue:

File: RewardsManager.sol Line 549

updatedRewards_ = rewardsCap - rewardsClaimedInEpoch;

In the code linked above the operation cannot underflow because of the check in line 546 ensures that we always have amount > amountToDecrement, so the operation should be marked as unchecked to save gas.

File: RewardsManager.sol Line 827

updatedRewards_ = rewardsCap - rewardsClaimedInEpoch;

In the code linked above the operation cannot underflow because of the check in line 723 ensures that we always have amount > amountToDecrement, so the operation should be marked as unchecked to save gas.

File: StandardFunding.sol Line 666

voter_.remainingVotingPower = votingPower - cumulativeVotePowerUsed;

In the code linked above the operation cannot underflow because of the check in line 663 ensures that we always have amount > amountToDecrement, so the operation should be marked as unchecked to save gas.

4- Use calldata instead of memory for function parameters type :

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

There are 13 instances of this issue:

File: RewardsManager.sol

135     function moveStakedLiquidity(
            uint256 tokenId_,
            uint256[] memory fromBuckets_,
            uint256[] memory toBuckets_,
            uint256 expiry_
        )
426     function _calculateNextEpochRewards(
            uint256 tokenId_,
            uint256 epoch_,
            uint256 stakingEpoch_,
            address ajnaPool_,
            uint256[] memory positionIndexes_
        )
671     function _updateBucketExchangeRates(
            address pool_,
            uint256[] memory indexes_
        )

File: GrantFund.sol

22      function hashProposal(
            address[] memory targets_,
            uint256[] memory values_,
            bytes[] memory calldatas_,
            bytes32 descriptionHash_
        )

File: ExtraordinaryFunding.sol

56      function executeExtraordinary(
            address[] memory targets_,
            uint256[] memory values_,
            bytes[] memory calldatas_,
            bytes32 descriptionHash_
        )
85      function proposeExtraordinary(
            uint256 endBlock_,
            address[] memory targets_,
            uint256[] memory values_,
            bytes[] memory calldatas_,
            string memory description_)

File: Funding.sol

52      function _execute(
            uint256 proposalId_,
            address[] memory targets_,
            uint256[] memory values_,
            bytes[] memory calldatas_
        )
103     function _validateCallDatas(
            address[] memory targets_,
            uint256[] memory values_,
            bytes[] memory calldatas_
        )
152     function _hashProposal(
            address[] memory targets_,
            uint256[] memory values_,
            bytes[] memory calldatas_,
            bytes32 descriptionHash_
        )

File: StandardFunding.sol

343     function executeStandard(
            address[] memory targets_,
            uint256[] memory values_,
            bytes[] memory calldatas_,
            bytes32 descriptionHash_
        ) 
366     function proposeStandard(
            address[] memory targets_,
            uint256[] memory values_,
            bytes[] memory calldatas_,
            string memory description_
        )
519     function fundingVote(
            FundingVoteParams[] memory voteParams_
        )
572     function screeningVote(
            ScreeningVoteParams[] memory voteParams_
        )

5- Multiple address/IDs mappings can be combined into a single mapping of an address/id to a struct :

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~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.

There are 13 instances of this issue :

File: PositionManager.sol Line 51-59

52:     mapping(uint256 => address) public override poolKey;
55:     mapping(uint256 => mapping(uint256 => Position)) internal positions;
57:     mapping(uint256 => uint96)                       internal nonces;
59:     mapping(uint256 => EnumerableSet.UintSet)        internal positionIndexes;

These mappings could be refactored into the following struct and mapping for example (this also enables to pack variables inside the struct to save gas) :

struct Pool {
    address poolKey;
    uint96 nonces;
    EnumerableSet.UintSet positionIndexes;
    mapping(uint256 => Position) positions;
}
    
mapping(uint256 => Pool) public tokenToPool;

File: RewardsManager.sol

70:     mapping(uint256 => mapping(uint256 => bool)) public override isEpochClaimed;
80:     mapping(uint256 => StakeInfo) internal stakes;

These mappings could be refactored into the following struct and mapping for example :

struct TokenIdStruct {
    StakeInfo stakes;
    mapping(uint256 => bool) isEpochClaimed;
}
    
mapping(uint256 => TokenIdStruct) public tokenIdMap;

File: ExtraordinaryFunding.sol

38:     mapping(uint256 => ExtraordinaryFundingProposal) internal _extraordinaryFundingProposals;
49:     mapping(uint256 => mapping(address => bool)) public hasVotedExtraordinary;

These mappings could be refactored into the following struct and mapping for example :

struct Proposal {
    ExtraordinaryFundingProposal _extraordinaryFundingProposals;
    mapping(address => bool) hasVotedExtraordinary;
}
    
mapping(uint256 => Proposal) public proposalIdMap;

File: StandardFunding.sol

82:     mapping(uint256 => uint256[]) internal _topTenProposals;
94:     mapping(uint256 => mapping(address => QuadraticVoter)) internal _quadraticVoters;
100:    mapping(uint256 => bool) internal _isSurplusFundsUpdated;
106:    mapping(uint256 => mapping(address => bool)) public hasClaimedReward;
112:    mapping(uint256 => mapping(address => uint256)) public screeningVotesCast;

These mappings could be refactored into the following struct and mapping for example :

struct Distribution {
    bool _isSurplusFundsUpdated;
    uint256[] _topTenProposals;
    mapping(address => QuadraticVoter) _quadraticVoters;
    mapping(address => bool) hasClaimedReward;
    mapping(address => uint256) screeningVotesCast;
}
    
mapping(uint256 => Distribution) public distributionIdMap;

6- x += y/x -= y costs more gas than x = x + y/x = x - y for state variables :

Using the addition (substraction) operator instead of plus-equals(plus-minus saves 113 gas as explained here

There are 10 instances of this issue:

File: StandardFunding.sol

157     treasury -= gbc;
217     treasury += (fundsAvailable - totalTokensRequested);
673     currentDistribution_.fundingVotePowerCast += incrementalVotingPowerUsed;
676     proposal_.fundingVotesReceived += SafeCast.toInt128(voteParams_.votesUsed);
712     proposal_.votesReceived += SafeCast.toUint128(votes_);

File: ExtraordinaryFunding.sol

78      treasury -= tokensRequested;
145     proposal.votesReceived += SafeCast.toUint120(votesCast_);

File: GrantFund.sol

62      treasury += fundingAmount_;

File: PositionManager.sol

320     fromPosition.lps -= vars.lpbAmountFrom;
321     toPosition.lps   += vars.lpbAmountTo;