Report generated by IllIllI-bot (bot crew: IllIllI)
| Issue | Instances | |
|---|---|---|
| [M‑01] | Use of transferFrom() rather than safeTransferFrom() for NFTs in will lead to the loss of NFTs |
1 |
| [M‑02] | _safeMint() should be used rather than _mint() wherever possible |
1 |
Total: 2 instances over 2 issues
| Issue | Instances | |
|---|---|---|
| [L‑01] | Loss of precision | 1 |
| [L‑02] | Array lengths not checked | 5 |
Total: 6 instances over 2 issues
| Issue | Instances | |
|---|---|---|
| [N‑01] | The nonReentrant modifier should occur before all other modifiers |
1 |
| [N‑02] | require()/revert() statements should have descriptive reason strings |
1 |
| [N‑03] | External calls in an un-bounded for-loop may result in a DOS |
1 |
| [N‑04] | constants should be defined rather than using magic numbers |
15 |
| [N‑05] | Numeric values having to do with time should use time units for readability | 3 |
| [N‑06] | Events that mark critical parameter changes should contain both the old and the new value | 1 |
| [N‑07] | Expressions for constant values such as a call to keccak256(), should use immutable rather than constant |
2 |
| [N‑08] | Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18) |
5 |
| [N‑09] | Use @inheritdoc rather than using a non-standard annotation |
1 |
| [N‑10] | Inconsistent spacing in comments | 4 |
| [N‑11] | Lines are too long | 70 |
| [N‑12] | Typos | 16 |
| [N‑13] | NatSpec @param is missing |
5 |
| [N‑14] | NatSpec @return argument is missing |
1 |
| [N‑15] | Event is not properly indexed |
1 |
| [N‑16] | Consider using delete rather than assigning zero to clear values |
1 |
| [N‑17] | Contracts should have full test coverage | 1 |
| [N‑18] | Large or complicated code bases should implement invariant tests | 1 |
| [N‑19] | Function ordering does not follow the Solidity style guide | 7 |
| [N‑20] | Contract does not follow the Solidity style guide's suggested layout ordering | 1 |
Total: 138 instances over 20 issues
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [G‑01] | Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate |
2 | - |
| [G‑02] | Using storage instead of memory for structs/arrays saves gas |
4 | 16800 |
| [G‑03] | State variables should be cached in stack variables rather than re-reading them from storage | 1 | 97 |
| [G‑04] | Multiple accesses of a mapping/array should use a local variable cache | 24 | 1008 |
| [G‑05] | internal functions only called once can be inlined to save gas |
7 | 140 |
| [G‑06] | Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement |
1 | 85 |
| [G‑07] | <array>.length should not be looked up in every loop of a for-loop |
8 | 24 |
| [G‑08] | ++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 |
1 | 60 |
| [G‑09] | Optimize names to save gas | 3 | 66 |
| [G‑10] | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead |
4 | - |
| [G‑11] | Using private rather than public for constants, saves gas |
1 | - |
| [G‑12] | Division by two should use bit shifting | 4 | 80 |
| [G‑13] | Constructors can be marked payable |
2 | 42 |
Total: 62 instances over 13 issues with 18402 gas saved
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.
[M‑01] Use of transferFrom() rather than safeTransferFrom() for NFTs in will lead to the loss of NFTs
The EIP-721 standard says the following about transferFrom():
/// @notice Transfer ownership of an NFT -- THE CALLER IS RESPONSIBLE
/// TO CONFIRM THAT `_to` IS CAPABLE OF RECEIVING NFTS OR ELSE
/// THEY MAY BE PERMANENTLY LOST
/// @dev Throws unless `msg.sender` is the current owner, an authorized
/// operator, or the approved address for this NFT. Throws if `_from` is
/// not the current owner. Throws if `_to` is the zero address. Throws if
/// `_tokenId` is not a valid NFT.
/// @param _from The current owner of the NFT
/// @param _to The new owner
/// @param _tokenId The NFT to transfer
function transferFrom(address _from, address _to, uint256 _tokenId) external payable;https://github.com/ethereum/EIPs/blob/78e2c297611f5e92b6a5112819ab71f74041ff25/EIPS/eip-721.md?plain=1#L103-L113
Code must use the safeTransferFrom() flavor if it hasn't otherwise verified that the receiving address can handle it
There is one instance of this issue:
File: ajna-core/src/RewardsManager.sol
302: IERC721(address(positionManager)).transferFrom(address(this), msg.sender, tokenId_);
_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both OpenZeppelin and solmate have versions of this function
There is one instance of this issue:
File: ajna-core/src/PositionManager.sol
238: _mint(params_.recipient, tokenId_);
Division by large numbers may result in the result being zero, due to solidity not supporting fractions. Consider requiring a minimum amount for the numerator to ensure that it is always larger than the denominator
There is one instance of this issue:
File: ajna-grants/src/grants/libraries/Maths.sol
38: return (x * 10**18 + y / 2) / y;
If the length of the arrays are not required to be of the same length, user operations may not be fully executed due to a mismatch in the number of items iterated over, versus the number of items provided in the second array
There are 5 instances of this issue:
File: ajna-grants/src/grants/base/ExtraordinaryFunding.sol
56 function executeExtraordinary(
57 address[] memory targets_,
58 uint256[] memory values_,
59 bytes[] memory calldatas_,
60 bytes32 descriptionHash_
61: ) external nonReentrant override returns (uint256 proposalId_) {
85 function proposeExtraordinary(
86 uint256 endBlock_,
87 address[] memory targets_,
88 uint256[] memory values_,
89 bytes[] memory calldatas_,
90: string memory description_) external override returns (uint256 proposalId_) {
File: ajna-grants/src/grants/base/StandardFunding.sol
343 function executeStandard(
344 address[] memory targets_,
345 uint256[] memory values_,
346 bytes[] memory calldatas_,
347 bytes32 descriptionHash_
348: ) external nonReentrant override returns (uint256 proposalId_) {
366 function proposeStandard(
367 address[] memory targets_,
368 uint256[] memory values_,
369 bytes[] memory calldatas_,
370 string memory description_
371: ) external override returns (uint256 proposalId_) {
File: ajna-grants/src/grants/GrantFund.sol
22 function hashProposal(
23 address[] memory targets_,
24 uint256[] memory values_,
25 bytes[] memory calldatas_,
26 bytes32 descriptionHash_
27: ) external pure override returns (uint256 proposalId_) {
This is a best-practice to protect against reentrancy in other modifiers
There is one instance of this issue:
File: ajna-core/src/PositionManager.sol
264: ) external override mayInteract(params_.pool, params_.tokenId) nonReentrant {
There is one instance of this issue:
File: ajna-core/src/PositionManager.sol
520: require(_exists(tokenId_));
Consider limiting the number of iterations in for-loops that make external calls
There is one instance of this issue:
File: ajna-core/src/RewardsManager.sol
/// @audit bucketExchangeRate
229: for (uint256 i = 0; i < positionIndexes.length; ) {
Even assembly can benefit from using readable constants instead of hex/numeric literals
There are 15 instances of this issue:
File: ajna-grants/src/grants/base/ExtraordinaryFunding.sol
/// @audit 0.5
/// @audit 1e18
209: return 0.5 * 1e18;
/// @audit 0.5
/// @audit 1e18
/// @audit 0.05
213: return 0.5 * 1e18 + (_fundedExtraordinaryProposals.length * (0.05 * 1e18));
File: ajna-grants/src/grants/base/Funding.sol
/// @audit 0x20
123: selector := mload(add(selDataWithSig, 0x20))
/// @audit 0xa9059cbb
125: if (selector != bytes4(0xa9059cbb)) revert InvalidProposal();
/// @audit 68
133: tokensRequested := mload(add(tokenDataWithSig, 68))
File: ajna-grants/src/grants/base/StandardFunding.sol
/// @audit 9
391: if (newProposal.tokensRequested > (currentDistribution.fundsAvailable * 9 / 10)) revert InvalidProposal();
/// @audit 9
448: if (totalTokensRequested > (gbc * 9 / 10)) {
File: ajna-grants/src/grants/libraries/Maths.sol
/// @audit 3
19: if (y > 3) {
/// @audit 9
30: z = z * 10**9;
/// @audit 18
34: return (x * y + 10**18 / 2) / 10**18;
/// @audit 18
38: return (x * 10**18 + y / 2) / y;
/// @audit 18
47: z = n % 2 != 0 ? x : 10**18;
There are units for seconds, minutes, hours, days, and weeks, and since they're defined, they should be used
There are 3 instances of this issue:
File: ajna-grants/src/grants/base/StandardFunding.sol
/// @audit 50400
29 /**
30 * @notice Length of the challengephase of the distribution period in blocks.
31 * @dev Roughly equivalent to the number of blocks in 7 days.
32 * @dev The period in which funded proposal slates can be checked in updateSlate.
33 */
34: uint256 internal constant CHALLENGE_PERIOD_LENGTH = 50400;
/// @audit 648000
36 /**
37 * @notice Length of the distribution period in blocks.
38 * @dev Roughly equivalent to the number of blocks in 90 days.
39 */
40: uint48 internal constant DISTRIBUTION_PERIOD_LENGTH = 648000;
/// @audit 72000
42 /**
43 * @notice Length of the funding phase of the distribution period in blocks.
44 * @dev Roughly equivalent to the number of blocks in 10 days.
45 */
46: uint256 internal constant FUNDING_PERIOD_LENGTH = 72000;
This should especially be done if the new value is not required to be different from the old value
There is one instance of this issue:
File: ajna-grants/src/grants/base/StandardFunding.sol
/// @audit updateSlate()
335 emit FundedSlateUpdated(
336 distributionId_,
337 newSlateHash
338: );
[N‑07] Expressions for constant values such as a call to keccak256(), should use immutable rather than constant
While it doesn't save any gas because the compiler knows that developers often make this mistake, it's still best to use the right tool for the task at hand. There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts. constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.
There are 2 instances of this issue:
File: ajna-grants/src/grants/base/ExtraordinaryFunding.sol
28: bytes32 internal constant DESCRIPTION_PREFIX_HASH_EXTRAORDINARY = keccak256(bytes("Extraordinary Funding: "));
File: ajna-grants/src/grants/base/StandardFunding.sol
51: bytes32 internal constant DESCRIPTION_PREFIX_HASH_STANDARD = keccak256(bytes("Standard Funding: "));
While the compiler knows to optimize away the exponentiation, it's still better coding practice to use idioms that do not require compiler optimization, if they exist
There are 5 instances of this issue:
File: ajna-grants/src/grants/libraries/Maths.sol
6: uint256 public constant WAD = 10**18;
30: z = z * 10**9;
34: return (x * y + 10**18 / 2) / 10**18;
38: return (x * 10**18 + y / 2) / y;
47: z = n % 2 != 0 ? x : 10**18;
There is one instance of this issue:
File: ajna-core/src/PositionManager.sol
514 /**
515 * @dev See {IERC721Metadata-tokenURI}.
516 */
517 function tokenURI(
518 uint256 tokenId_
519: ) public view override(ERC721) returns (string memory) {
Some lines use // x and some use //x. The instances below point out the usages that don't follow the majority, within each file
There are 4 instances of this issue:
File: ajna-grants/src/grants/base/Funding.sol
121: //slither-disable-next-line assembly
131: //slither-disable-next-line assembly
File: ajna-grants/src/grants/base/StandardFunding.sol
771: //slither-disable-next-line incorrect-equality
798: //slither-disable-next-line incorrect-equality
Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. The solidity style guide recommends a maximumum line length of 120 characters, so the lines below should be split when they reach that length.
There are 70 instances of this issue:
File: ajna-core/src/PositionManager.sol
423: address erc20DeployedPoolAddress = erc20PoolFactory.deployedPools(subsetHash_, collateralAddress, quoteAddress);
424: address erc721DeployedPoolAddress = erc721PoolFactory.deployedPools(subsetHash_, collateralAddress, quoteAddress);
File: ajna-core/src/RewardsManager.sol
53: * @dev ensures that rewards issued to staked lenders in a given pool are less than the `Ajna` tokens burned in that pool.
76: /// @dev Mapping of per pool bucket exchange rates at a given burn event `poolAddress => bucketIndex => epoch => bucket exchange rate`.
168: IPositionManagerOwnerActions.MoveLiquidityParams memory moveLiquidityParams = IPositionManagerOwnerActions.MoveLiquidityParams(
379: * @dev Rewards are calculated as the difference in exchange rates between the last interaction burn event and the current burn event.
381: * @param epochToClaim_ The burn epoch to claim rewards for (rewards calculation starts from the last claimed epoch).
418: * @dev Rewards are calculated as the difference in exchange rates between the last interaction burn event and the current burn event.
557: * @param epochToClaim_ The burn epoch to claim rewards for (rewards calculation starts from the last claimed epoch)
558: * @param validateEpoch_ True if the epoch is received as a parameter and needs to be validated (lower or equal with latest epoch).
653: uint256 totalBurned = totalBurnedLatest != 0 ? totalBurnedLatest - totalBurnedAtBlock : totalBurnedAtBlock;
654: uint256 totalInterest = totalInterestLatest != 0 ? totalInterestLatest - totalInterestAtBlock : totalInterestAtBlock;
665: * @dev Called as part of `stake`, `unstake`, and `claimRewards`, as well as `updateBucketExchangeRatesAndClaim`.
666: * @dev Caller can claim `5%` of the rewards that have accumulated to each bucket since the last burn event, if it hasn't already been updated.
787: // skip reward calculation if update at the previous epoch was missed and if exchange rate decreased due to bad debt
788: // prevents excess rewards from being provided from using a 0 value as an input to the interestFactor calculation below.
808: * @dev It is used to ensure that rewards claimers will be able to claim some portion of the remaining tokens if a claim would exceed the remaining contract balance.
File: ajna-grants/src/grants/base/ExtraordinaryFunding.sol
62: proposalId_ = _hashProposal(targets_, values_, calldatas_, keccak256(abi.encode(DESCRIPTION_PREFIX_HASH_EXTRAORDINARY, descriptionHash_)));
70: if (proposal.executed || !_extraordinaryProposalSucceeded(proposalId_, tokensRequested)) revert ExecuteExtraordinaryProposalInvalid();
92: proposalId_ = _hashProposal(targets_, values_, calldatas_, keccak256(abi.encode(DESCRIPTION_PREFIX_HASH_EXTRAORDINARY, keccak256(bytes(description_)))));
105: if (uint256(totalTokensRequested) > _getSliceOfTreasury(Maths.WAD - _getMinimumThresholdPercentage())) revert InvalidProposal();
File: ajna-grants/src/grants/base/Funding.sol
48: * @param targets_ The list of smart contract targets for the calldata execution. Should be the Ajna token address.
70: * @dev Voting power is the minimum of the amount of votes available at a snapshot block 33 blocks prior to voting start, and at the vote starting block.
72: * @param snapshot_ One of the block numbers to retrieve the voting power at. 33 blocks prior to the block at which a proposal is available for voting.
110: if (targets_.length == 0 || targets_.length != values_.length || targets_.length != calldatas_.length) revert InvalidProposal();
144: * @notice Create a proposalId from a hash of proposal's targets, values, and calldatas arrays, and a description hash.
149: * @param descriptionHash_ The hash of the proposal's description string. Generated by keccak256(bytes(description))).
File: ajna-grants/src/grants/base/StandardFunding.sol
269: * @dev Voter must have voted in both the screening and funding stages, and is proportional to their share of votes across the stages.
310: uint256 sum = _validateSlate(distributionId_, currentDistribution.endBlock, currentDistribution.fundsAvailable, proposalIds_, numProposalsInSlate);
349: proposalId_ = _hashProposal(targets_, values_, calldatas_, keccak256(abi.encode(DESCRIPTION_PREFIX_HASH_STANDARD, descriptionHash_)));
354: // check that the distribution period has ended, and one week has passed to enable competing slates to be checked
355: if (block.number <= _getChallengeStageEndBlock(_distributions[distributionId].endBlock)) revert ExecuteProposalInvalid();
372: proposalId_ = _hashProposal(targets_, values_, calldatas_, keccak256(abi.encode(DESCRIPTION_PREFIX_HASH_STANDARD, keccak256(bytes(description_)))));
388: newProposal.tokensRequested = _validateCallDatas(targets_, values_, calldatas_); // check proposal parameters are valid and update tokensRequested
411: * @notice Check the validity of a potential slate of proposals to execute, and sum the slate's fundingVotesReceived.
419: * @return sum_ The total funding votes received by all proposals in the proposed slate.
421: function _validateSlate(uint24 distributionId_, uint256 endBlock, uint256 distributionPeriodFundsAvailable_, uint256[] calldata proposalIds_, uint256 numProposalsInSlate_) internal view returns (uint256 sum_) {
438: if (_findProposalIndex(proposalIds_[i], _topTenProposals[distributionId_]) == -1) revert InvalidProposalSlate();
444: sum_ += uint128(proposal.fundingVotesReceived); // since we are converting from int128 to uint128, we can safely assume that the value will not overflow
541: uint128 newVotingPower = SafeCast.toUint128(_getVotesFunding(msg.sender, votingPower, voter.remainingVotingPower, screeningStageEndBlock));
556: if (_findProposalIndex(voteParams_[i].proposalId, _topTenProposals[currentDistributionId]) == -1) revert InvalidVote();
578: if (block.number < currentDistribution.startBlock || block.number > _getScreeningStageEndBlock(currentDistribution.endBlock)) revert InvalidVote();
609: * @param voteParams_ The amount of votes being allocated to the proposal. Not squared. If less than 0, vote is against.
668: // calculate the change in voting power used by the voter in this vote in order to accurately track the total voting power used in the funding stage
682: // emits the amount of incremental votes cast for the proposal, not the voting power cost or total votes on a proposal
706: if (screeningVotesCast[distributionId][account_] + votes_ > _getVotesScreening(distributionId, account_)) revert InsufficientVotingPower();
732: else if(_standardFundingProposals[currentTopTenProposals[screenedProposalsLength - 1]].votesReceived < proposal_.votesReceived) {
823: _standardFundingProposals[proposals_[targetProposalId_]].votesReceived > _standardFundingProposals[proposals_[targetProposalId_ - 1]].votesReceived
864: return _findProposalIndex(proposalId_, _fundedProposalSlates[_distributions[distributionId].fundedSlateHash]) != -1;
886: * @param votingPower_ The voter's voting power in the funding round. Equal to the square of their tokens in the voting snapshot.
887: * @param remainingVotingPower_ The voter's remaining quadratic voting power in the given distribution period's funding round.
961: function getFundingVotesCast(uint24 distributionId_, address account_) external view override returns (FundingVoteParams[] memory) {
File: ajna-grants/src/grants/GrantFund.sol
50: return mechanism == FundingMechanism.Standard ? _standardProposalState(proposalId_) : _getExtraordinaryProposalState(proposalId_);
File: ajna-grants/src/grants/interfaces/IGrantFund.sol
31: * @notice Create a proposalId from a hash of proposal's targets, values, and calldatas arrays, and a description hash.
36: * @param descriptionHash_ The hash of the proposal's description string. Generated by keccak256(bytes(description))).
File: ajna-grants/src/grants/interfaces/IStandardFunding.sol
132: * @notice Contains information about voters during a vote made by a QuadraticVoter in the Funding stage of a distribution period.
152: uint128 votingPower; // amount of votes originally available to the voter, equal to the sum of the square of their initial votes
190: * @param targets_ List of contracts the proposal calldata will interact with. Should be the Ajna token contract for all proposals.
205: * @dev All proposals can be submitted by anyone. There can only be one value in each array. Interface inherits from OZ.propose().
206: * @param targets_ List of contracts the proposal calldata will interact with. Should be the Ajna token contract for all proposals.
220: * @notice Check if a slate of proposals meets requirements, and maximizes votes. If so, update QuarterlyDistribution.
223: * @return newTopSlate_ Boolean indicating whether the new proposal slate was set as the new top slate for distribution.
318: function getFundingVotesCast(uint24 distributionId_, address account_) external view returns (FundingVoteParams[] memory);
325: * @return votesReceived The amount of votes the proposal has received in it's distribution period's screening round.
327: * @return qvBudgetAllocated The amount of quadratic vote budget allocated to the proposal in it's distribution period's funding round.
344: * @notice Retrieve the top ten proposals that have received the most votes in a given distribution period's screening round.
358: * @return votingPower The voter's voting power in the funding round. Equal to the square of their tokens in the voting snapshot.
359: * @return remainingVotingPower The voter's remaining quadratic voting power in the given distribution period's funding round.
368: * @notice Get the remaining quadratic voting power available to the voter in the funding stage of a distribution period.
File: ajna-grants/src/grants/libraries/Maths.sol
14: * @dev Utilizes the babylonian method: https://en.wikipedia.org/wiki/Methods_of_computing_square_roots#Babylonian_method.
There are 16 instances of this issue:
File: ajna-core/src/PositionManager.sol
/// @audit bucekt
83: uint256 depositTime; // lender deposit time in from bucekt
/// @audit memorialzied
434: * @return `True` if the bucket went bankrupt after that position memorialzied their `LP`.
File: ajna-core/src/RewardsManager.sol
/// @audit aready
189: // update to bucket list exchange rates, from buckets are aready updated on claim
File: ajna-grants/src/grants/base/ExtraordinaryFunding.sol
/// @audit succesful
69: // check proposal is succesful and hasn't already been executed
/// @audit compatability
186: * @dev Used by GrantFund.state() for analytics compatability purposes.
File: ajna-grants/src/grants/base/StandardFunding.sol
/// @audit calculat
270: * @param currentDistribution_ Struct of the distribution period to calculat rewards for.
/// @audit succesful
357: // check proposal is succesful and hasn't already been executed
/// @audit compatability
501: * @dev Used by GrantFund.state() for analytics compatability purposes.
File: ajna-grants/src/grants/interfaces/IExtraordinaryFunding.sol
/// @audit succesful
122: * @return executed Whether a succesful proposal has been executed.
File: ajna-grants/src/grants/interfaces/IFunding.sol
/// @audit paramteres
21: * @notice User submitted a proposal with invalid paramteres.
/// @audit execcution
38: * @notice Proposal didn't meet requirements for execcution.
File: ajna-grants/src/grants/interfaces/IStandardFunding.sol
/// @audit distrubtions
82: * @param startBlock Block number of the quarterly distrubtions start.
/// @audit distrubtions
83: * @param endBlock Block number of the quarterly distrubtions end.
/// @audit whinch
175: * @param distributionId_ Id of distribution from whinch delegatee wants to claim his reward.
/// @audit succesfully
189: * @dev Check for proposal being succesfully funded or previously executed is handled by Governor.execute().
/// @audit succesfully
316: * @return FundingVoteParams The list of FundingVoteParams structs that have been succesfully cast the voter.
There are 5 instances of this issue:
File: ajna-grants/src/grants/base/ExtraordinaryFunding.sol
/// @audit Missing: '@param tokensRequested_'
159 /**
160 * @notice Check if a proposal for extraordinary funding has succeeded.
161 * @param proposalId_ The ID of the proposal being checked.
162 * @return Boolean indicating whether the proposal has succeeded.
163 */
164 function _extraordinaryProposalSucceeded(
165 uint256 proposalId_,
166 uint256 tokensRequested_
167: ) internal view returns (bool) {
File: ajna-grants/src/grants/base/Funding.sol
/// @audit Missing: '@param proposalId_'
46 /**
47 * @notice Execute the calldata of a passed proposal.
48 * @param targets_ The list of smart contract targets for the calldata execution. Should be the Ajna token address.
49 * @param values_ Unused. Should be 0 since all calldata is executed on the Ajna token's transfer method.
50 * @param calldatas_ The list of calldatas to execute.
51 */
52 function _execute(
53 uint256 proposalId_,
54 address[] memory targets_,
55 uint256[] memory values_,
56: bytes[] memory calldatas_
File: ajna-grants/src/grants/base/StandardFunding.sol
/// @audit Missing: '@param distributionId_'
867 /**
868 * @notice Retrieve the number of votes available to an account in the current screening stage.
869 * @param account_ The account to retrieve votes for.
870 * @return votes_ The number of votes available to an account in this screening stage.
871 */
872: function _getVotesScreening(uint24 distributionId_, address account_) internal view returns (uint256 votes_) {
File: ajna-grants/src/grants/libraries/Maths.sol
/// @audit Missing: '@param x'
/// @audit Missing: '@param n'
45 /** @notice Raises a WAD to the power of an integer and returns a WAD */
46: function wpow(uint256 x, uint256 n) internal pure returns (uint256 z) {
There is one instance of this issue:
File: ajna-grants/src/grants/libraries/Maths.sol
/// @audit Missing: '@return'
45 /** @notice Raises a WAD to the power of an integer and returns a WAD */
46: function wpow(uint256 x, uint256 n) internal pure returns (uint256 z) {
Index event fields make the field more quickly accessible to off-chain tools that parse events. This is especially useful when it comes to filtering based on an address. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Where applicable, each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three applicable fields, all of the applicable fields should be indexed.
There is one instance of this issue:
File: ajna-grants/src/grants/interfaces/IFunding.sol
54 event ProposalCreated(
55 uint256 proposalId,
56 address proposer,
57 address[] targets,
58 uint256[] values,
59 string[] signatures,
60 bytes[] calldatas,
61 uint256 startBlock,
62 uint256 endBlock,
63 string description
64: );
The delete keyword more closely matches the semantics of what is being done, and draws more attention to the changing of state, which may lead to a more thorough audit of its associated logic
There is one instance of this issue:
File: ajna-core/src/PositionManager.sol
197: position.lps = 0;
While 100% code coverage does not guarantee that there are no bugs, it often will catch easy-to-find bugs, and will ensure that there are fewer regressions when the code invariably has to be modified. Furthermore, in order to get full coverage, code authors will often have to re-organize their code so that it is more modular, so that each component can be tested separately, which reduces interdependencies between modules and layers, and makes for code that is easier to reason about and audit.
There is one instance of this issue:
File: Various Files
Large code bases, or code with lots of inline-assembly, complicated math, or complicated interactions between multiple contracts, should implement invariant fuzzing tests. Invariant fuzzers such as Echidna require the test writer to come up with invariants which should not be violated under any circumstances, and the fuzzer tests various inputs and function calls to ensure that the invariants always hold. Even code with 100% code coverage can still have bugs due to the order of the operations a user performs, and invariant fuzzers, with properly and extensively-written invariants, can close this testing gap significantly.
There is one instance of this issue:
File: Various Files
According to the Solidity style guide, functions should be laid out in the following order :constructor(), receive(), fallback(), external, public, internal, private, but the cases below do not follow this pattern
There are 7 instances of this issue:
File: ajna-core/src/PositionManager.sol
/// @audit _bucketBankruptAfterDeposit() came earlier
450 function getLP(
451 uint256 tokenId_,
452 uint256 index_
453: ) external override view returns (uint256) {
File: ajna-grants/src/grants/base/ExtraordinaryFunding.sol
/// @audit _getVotesExtraordinary() came earlier
263: function getMinimumThresholdPercentage() external view returns (uint256) {
File: ajna-grants/src/grants/base/StandardFunding.sol
/// @audit _setNewDistributionId() came earlier
236 function claimDelegateReward(
237 uint24 distributionId_
238: ) external override returns(uint256 rewardClaimed_) {
/// @audit _getDelegateReward() came earlier
300 function updateSlate(
301 uint256[] calldata proposalIds_,
302 uint24 distributionId_
303: ) external override returns (bool newTopSlate_) {
/// @audit _standardProposalState() came earlier
519 function fundingVote(
520 FundingVoteParams[] memory voteParams_
521: ) external override returns (uint256 votesCast_) {
/// @audit _getVotesFunding() came earlier
917 function getDelegateReward(
918 uint24 distributionId_,
919 address voter_
920: ) external view override returns (uint256 rewards_) {
File: ajna-grants/src/grants/GrantFund.sol
/// @audit findMechanismOfProposal() came earlier
45 function state(
46 uint256 proposalId_
47: ) external view override returns (ProposalState) {
The style guide says that, within a contract, the ordering should be 1) Type declarations, 2) State variables, 3) Events, 4) Modifiers, and 5) Functions, but the contract(s) below do not follow this ordering
There is one instance of this issue:
File: ajna-grants/src/grants/interfaces/IFunding.sol
/// @audit event VoteCast came earlier
78 enum FundingMechanism {
79 Standard,
80 Extraordinary
81: }
[G‑01] Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate
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 2 instances of this issue:
File: ajna-core/src/PositionManager.sol
54 /// @dev Mapping of `token id => ajna pool address` for which token was minted.
55 mapping(uint256 => mapping(uint256 => Position)) internal positions;
56 /// @dev Mapping of `token id => nonce` value used for permit.
57 mapping(uint256 => uint96) internal nonces;
58 /// @dev Mapping of `token id => bucket indexes` associated with position.
59: mapping(uint256 => EnumerableSet.UintSet) internal positionIndexes;
File: ajna-core/src/RewardsManager.sol
71 /// @dev `epoch => rewards claimed` mapping.
72 mapping(uint256 => uint256) public override rewardsClaimed;
73 /// @dev `epoch => update bucket rate rewards claimed` mapping.
74: mapping(uint256 => uint256) public override updateRewardsClaimed;
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 4 instances of this issue:
File: ajna-grants/src/grants/base/StandardFunding.sol
203: uint256[] memory fundingProposalIds = _fundedProposalSlates[fundedSlateHash];
209: Proposal memory proposal = _standardFundingProposals[fundingProposalIds[i]];
379: QuarterlyDistribution memory currentDistribution = _distributions[_currentDistributionId];
575: QuarterlyDistribution memory currentDistribution = _distributions[_currentDistributionId];
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
There is one instance of this issue:
File: ajna-grants/src/grants/base/ExtraordinaryFunding.sol
/// @audit _fundedExtraordinaryProposals on line 208
213: return 0.5 * 1e18 + (_fundedExtraordinaryProposals.length * (0.05 * 1e18));
The instances below 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
There are 24 instances of this issue:
File: ajna-core/src/RewardsManager.sol
/// @audit stakes[tokenId_] on line 330
331: uint256 lastClaimedEpoch = stakes[tokenId_].lastClaimedEpoch;
/// @audit stakes[tokenId_] on line 331
332: uint256 stakingEpoch = stakes[tokenId_].stakingEpoch;
/// @audit stakes[tokenId_] on line 356
357: stakes[tokenId_].ajnaPool,
/// @audit stakes[tokenId_] on line 357
358: stakes[tokenId_].lastClaimedEpoch
/// @audit stakes[tokenId_] on line 368
369: stakes[tokenId_].snapshot[bucketId_].rateAtStakeTime
/// @audit stakes[tokenId_] on line 389
390: uint256 lastClaimedEpoch = stakes[tokenId_].lastClaimedEpoch;
/// @audit stakes[tokenId_] on line 390
391: uint256 stakingEpoch = stakes[tokenId_].stakingEpoch;
File: ajna-grants/src/grants/base/ExtraordinaryFunding.sol
/// @audit _extraordinaryFundingProposals[proposalId_] on line 286
287: _extraordinaryFundingProposals[proposalId_].startBlock,
/// @audit _extraordinaryFundingProposals[proposalId_] on line 287
288: _extraordinaryFundingProposals[proposalId_].endBlock,
/// @audit _extraordinaryFundingProposals[proposalId_] on line 288
289: _extraordinaryFundingProposals[proposalId_].tokensRequested,
/// @audit _extraordinaryFundingProposals[proposalId_] on line 289
290: _extraordinaryFundingProposals[proposalId_].votesReceived,
/// @audit _extraordinaryFundingProposals[proposalId_] on line 290
291: _extraordinaryFundingProposals[proposalId_].executed
File: ajna-grants/src/grants/base/StandardFunding.sol
/// @audit _distributions[distributionId_] on line 200
201: uint256 fundsAvailable = _distributions[distributionId_].fundsAvailable;
/// @audit _standardFundingProposals[<etc>] on line 823
823: _standardFundingProposals[proposals_[targetProposalId_]].votesReceived > _standardFundingProposals[proposals_[targetProposalId_ - 1]].votesReceived
/// @audit _distributions[distributionId_] on line 937
938: _distributions[distributionId_].startBlock,
/// @audit _distributions[distributionId_] on line 938
939: _distributions[distributionId_].endBlock,
/// @audit _distributions[distributionId_] on line 939
940: _distributions[distributionId_].fundsAvailable,
/// @audit _distributions[distributionId_] on line 940
941: _distributions[distributionId_].fundingVotePowerCast,
/// @audit _distributions[distributionId_] on line 941
942: _distributions[distributionId_].fundedSlateHash
/// @audit _standardFundingProposals[proposalId_] on line 970
971: _standardFundingProposals[proposalId_].distributionId,
/// @audit _standardFundingProposals[proposalId_] on line 971
972: _standardFundingProposals[proposalId_].votesReceived,
/// @audit _standardFundingProposals[proposalId_] on line 972
973: _standardFundingProposals[proposalId_].tokensRequested,
/// @audit _standardFundingProposals[proposalId_] on line 973
974: _standardFundingProposals[proposalId_].fundingVotesReceived,
/// @audit _standardFundingProposals[proposalId_] on line 974
975: _standardFundingProposals[proposalId_].executed
Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
There are 7 instances of this issue:
File: ajna-core/src/PositionManager.sol
416 function _isAjnaPool(
417 address pool_,
418 bytes32 subsetHash_
419: ) internal view returns (bool) {
File: ajna-core/src/RewardsManager.sol
384 function _calculateAndClaimRewards(
385 uint256 tokenId_,
386 uint256 epochToClaim_
387: ) internal returns (uint256 rewards_) {
487 function _calculateExchangeRateInterestEarned(
488 address pool_,
489 uint256 nextEventEpoch_,
490 uint256 bucketIndex_,
491 uint256 bucketLP_,
492 uint256 exchangeRate_
493: ) internal view returns (uint256 interestEarned_) {
519 function _calculateNewRewards(
520 address ajnaPool_,
521 uint256 interestEarned_,
522 uint256 nextEpoch_,
523 uint256 epoch_,
524 uint256 rewardsClaimedInEpoch_
525: ) internal view returns (uint256 newRewards_) {
606 function _getBurnEpochsClaimed(
607 uint256 lastClaimedEpoch_,
608 uint256 burnEpochToStartClaim_
609: ) internal pure returns (uint256[] memory burnEpochsClaimed_) {
743 function _updateBucketExchangeRate(
744 address pool_,
745 uint256 bucketIndex_,
746: uint256 burnEpoch_
768 function _updateBucketExchangeRateAndCalculateRewards(
769 address pool_,
770 uint256 bucketIndex_,
771 uint256 burnEpoch_,
772 uint256 totalBurned_,
773 uint256 interestEarned_
774: ) internal returns (uint256 rewards_) {
[G‑06] Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement
require(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }
There is one instance of this issue:
File: ajna-core/src/RewardsManager.sol
/// @audit if-condition on line 500
504: interestEarned_ = Maths.wmul(nextExchangeRate - exchangeRate_, bucketLP_);
The overheads outlined below are PER LOOP, excluding the first loop
- storage arrays incur a Gwarmaccess (100 gas)
- memory arrays use
MLOAD(3 gas) - calldata arrays use
CALLDATALOAD(3 gas)
Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
There are 8 instances of this issue:
File: ajna-core/src/RewardsManager.sol
229: for (uint256 i = 0; i < positionIndexes.length; ) {
290: for (uint256 i = 0; i < positionIndexes.length; ) {
440: for (uint256 i = 0; i < positionIndexes_.length; ) {
680: for (uint256 i = 0; i < indexes_.length; ) {
704: for (uint256 i = 0; i < indexes_.length; ) {
File: ajna-grants/src/grants/base/Funding.sol
62: for (uint256 i = 0; i < targets_.length; ++i) {
112: for (uint256 i = 0; i < targets_.length;) {
File: ajna-grants/src/grants/base/StandardFunding.sol
491: for (uint i = 0; i < proposalIdSubset_.length;) {
[G‑08] ++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
The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop
There is one instance of this issue:
File: ajna-grants/src/grants/base/Funding.sol
62: for (uint256 i = 0; i < targets_.length; ++i) {
public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted
There are 3 instances of this issue:
File: ajna-grants/src/grants/interfaces/IExtraordinaryFunding.sol
/// @audit executeExtraordinary(), proposeExtraordinary(), voteExtraordinary(), getSliceOfNonTreasury(), getSliceOfTreasury(), getExtraordinaryProposalInfo(), getExtraordinaryProposalSucceeded(), getMinimumThresholdPercentage(), getVotesExtraordinary()
9: interface IExtraordinaryFunding {
File: ajna-grants/src/grants/interfaces/IGrantFund.sol
/// @audit hashProposal(), state(), fundTreasury()
9: interface IGrantFund is
File: ajna-grants/src/grants/interfaces/IStandardFunding.sol
/// @audit startNewDistributionPeriod(), claimDelegateReward(), executeStandard(), proposeStandard(), updateSlate(), fundingVote(), screeningVote(), getDelegateReward(), getDistributionId(), getDistributionPeriodInfo(), getFundedProposalSlate(), getFundingPowerVotes(), getFundingVotesCast(), getProposalInfo(), getSlateHash(), getTopTenProposals(), getVoterInfo(), getVotesFunding(), getVotesScreening()
9: interface IStandardFunding {
When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html
Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) 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
There are 4 instances of this issue:
File: ajna-grants/src/grants/base/Funding.sol
/// @audit uint128 tokensRequested_
137: tokensRequested_ += SafeCast.toUint128(tokensRequested);
File: ajna-grants/src/grants/base/StandardFunding.sol
/// @audit uint24 newDistributionId_
146: newDistributionId_ = _setNewDistributionId();
/// @audit uint128 sum_
493: sum_ += uint128(_standardFundingProposals[proposalIdSubset_[i]].fundingVotesReceived);
/// @audit uint8 support
623: voteParams_.votesUsed < 0 ? support = 0 : support = 1;
If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table
There is one instance of this issue:
File: ajna-grants/src/grants/libraries/Maths.sol
6: uint256 public constant WAD = 10**18;
<x> / 2 is the same as <x> >> 1. While the compiler uses the SHR opcode to accomplish both, the version that uses division incurs an overhead of 20 gas due to JUMPs to and from a compiler utility function that introduces checks which can be avoided by using unchecked {} around the division by two
There are 4 instances of this issue:
File: ajna-grants/src/grants/libraries/Maths.sol
21: uint256 x = y / 2 + 1;
24: x = (y / x + x) / 2;
34: return (x * y + 10**18 / 2) / 10**18;
38: return (x * 10**18 + y / 2) / y;
Payable functions cost less gas to execute, since the compiler does not have to add extra checks to ensure that a payment wasn't provided. A constructor can safely be marked as payable, since only the deployer would be able to pass funds, and the project itself would not pass any funds.
There are 2 instances of this issue:
File: ajna-core/src/PositionManager.sol
116 constructor(
117 ERC20PoolFactory erc20Factory_,
118 ERC721PoolFactory erc721Factory_
119: ) PermitERC721("Ajna Positions NFT-V1", "AJNA-V1-POS", "1") {
File: ajna-core/src/RewardsManager.sol
95: constructor(address ajnaToken_, IPositionManager positionManager_) {
Judge comments:
[M‑01] Use of transferFrom() rather than safeTransferFrom() for NFTs in will lead to the loss of NFTs 1
L
[M‑02] _safeMint() should be used rather than _mint() wherever possible 1
L
[L‑01] Loss of precision 1
L
[L‑02] Array lengths not checked 5
R
[N‑01] The nonReentrant modifier should occur before all other modifiers 1
R
[N‑02] require()/revert() statements should have descriptive reason strings 1
NC
[N‑03] External calls in an un-bounded for-loop may result in a DOS 1
L
[N‑04] constants should be defined rather than using magic numbers 15
R
[N‑05] Numeric values having to do with time should use time units for readability 3
I
[N‑06] Events that mark critical parameter changes should contain both the old and the new value 1
NC
[N‑07] Expressions for constant values such as a call to keccak256(), should use immutable rather than constant 2
I
[N‑08] Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18) 5
NC
[N‑09] Use @inheritdoc rather than using a non-standard annotation 1
NC
[N‑10] Inconsistent spacing in comments 4
NC
[N‑11] Lines are too long 70
NC
[N‑12] Typos 16
NC
[N‑13] NatSpec @param is missing 5
NC
[N‑14] NatSpec @return argument is missing 1
13
[N‑15] Event is not properly indexed 1
R (proposalId and proposer)
[N‑16] Consider using delete rather than assigning zero to clear values 1
I
[N‑17] Contracts should have full test coverage 1
I
[N‑18] Large or complicated code bases should implement invariant tests 1
I
[N‑19] Function ordering does not follow the Solidity style guide 7
NC
[N‑20] Contract does not follow the Solidity style guide's suggested layout ordering 1
NC
Gas Optimizations
Issue Instances Total Gas Saved
[G‑01] Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate 2 -
NC
[G‑02] Using storage instead of memory for structs/arrays saves gas 4 16800
R
[G‑03] State variables should be cached in stack variables rather than re-reading them from storage 1 97
L
[G‑04] Multiple accesses of a mapping/array should use a local variable cache 24 1008
I
[G‑05] internal functions only called once can be inlined to save gas 7 140
I
[G‑06] Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement 1 85
NC
[G‑07] .length should not be looked up in every loop of a for-loop 8 24
NC
[G‑08] ++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 1 60
NC
[G‑09] Optimize names to save gas 3 66
I
[G‑10] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead 4 -
I
[G‑11] Using private rather than public for constants, saves gas 1 -
I
[G‑12] Division by two should use bit shifting 4 80
NC
[G‑13] Constructors can be marked payable
I
safeTransferfor ERC20 vs ERC721 was the reason I ended up awarding this bot the best