Platform: Code4rena
Start Date: 10/02/2022
Pot Size: $100,000 USDC
Total HM: 13
Participants: 21
Period: 7 days
Judge: leastwood
Total Solo HM: 10
Id: 85
League: ETH
Rank: 13/21
Findings: 1
Award: $564.53
🌟 Selected for report: 0
🚀 Solo Findings: 0
The local variable used as for loop index need not be initialized to 0 because the default value is 0. Avoiding this anti-pattern can save a few opcodes and therefore a tiny bit of gas.
for (uint256 i = 0; i < vars.datas.length; ++i) {
The same situation are in other scope contracts where loops use. Remix
Remove explicit 0 initialization of for loop index variable.
Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met.
There are several other places throughout the codebase where the same optimization can be used.
Shorten the revert strings to fit in 32 bytes.
!= 0 is a cheaper operation compared to > 0, when dealing with uint.
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/follow/ApprovalFollowModule.sol#L64 https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/base/ERC721Time.sol#L135
Remix
For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.
Remix
Consider using 'unchecked' where it is safe to do so.
A variable is being assigned its default value which is unnecessary. Removing the assignment will save gas when deploying.
uint256 private lastInitializedRevision = 0;
Remix
Remove the assignment.
Lower than uint256 size storage instance variables are actually less gas efficient. E.g. using uint24 does not give any efficiency, actually, it is the opposite as EVM operates on default of 256-bit values so uint24 is more expensive in this case as it needs a conversion. It only gives improvements in cases where you can pack variables together, e.g. structs.
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/collect/LimitedTimedFeeCollectModule.sol#L48 https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/libraries/Constants.sol#L10 https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/FeeModuleBase.sol#L17
uint24 internal constant ONE_DAY = 24 hours;
Remix
Consider to review all uint types. Change them with uint256 If the integer is not necessary to present with uint24.
In the Constants library, declaring the constants with type bytes32 can save gas.
https://medium.com/layerx/how-to-reduce-gas-cost-in-solidity-f2e5321e0395#2a78
whenNotPaused and function _validateNotPaused modifier whenNotPaused calls internal function _validateNotPaused. This function is not called anywhere else so I do not see a reason why all the logic can't be moved to the modifier to save some gas by reducing the extra call.
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/base/LensMultiState.sol#L18 The same in: https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/base/LensMultiState.sol#L23
modifier whenNotPaused() { _validateNotPaused(); _; }
Remix
Remove function _validateNotPaused, place its logic directly in the modifier whenNotPaused`.
Checking if adjustedAmount and treasuryAmount > 0 before making the external library call to IERC20(currency) can save gas by avoiding the external call in such situations.
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/collect/FeeCollectModule.sol#L135-L136 https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/collect/LimitedTimedFeeCollectModule.sol#L164-L165
There are several other places throughout the codebase where the same optimization can be used.
Remix
Change abi.encode to abi.encodePacked can save gas.
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/FollowNFT.sol#L86 https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/LensHub.sol#L185 There are several other places throughout the codebase where the same optimization can be used.
Remix
On external functions, when using the memory keyword with a function argument, what's happening is that a memory acts as an intermediate. Reading directly from calldata using calldataload instead of going via memory saves the gas from the intermediate memory operations that carry the values. As an extract from https://ethereum.stackexchange.com/questions/74442/when-should-i-use-calldata-and-when-should-i-use-memory : «memory and calldata (as well as storage) are keywords that define the data area where a variable is stored. To answer your question directly, memory should be used when declaring variables (both function parameters as well as inside the logic of a function) that you want stored in memory (temporary), and calldata must be used when declaring an external function's dynamic parameters. The easiest way to think about the difference is that calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.»
Remix
Use calldata instead of memory for external functions where the function argument is read-only.
Booleans are more expensive than uint256 or any type that takes up a full word because each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled.
Several internal functions are in contracts that are never inherited. Private functions are cheaper than internal functions. Therefore, their visibility should be reduced to private.
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/libraries/PublishingLogic.sol#L380 https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/FollowNFT.sol#L225
Remix
Define these functions as private.
Some of the variable can be cached to slightly reduce gas usage.
HUB can be cached.
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/FollowNFT.sol#L207-L212
Remix
Consider caching those variable for read and make sure write back to storage.
#0 - Zer0dot
2022-03-22T16:51:18Z
The point about unchecked is valid!
A lot of the rest is kind of valid, but for some clarifications:
GT opcode, whereas != uses EQ and NOT, so 3 extra gas.#1 - Zer0dot
2022-03-22T17:19:31Z
Actually on the treasury fee point, after some discussion, I think it may be valid to implement, linking issue #62 in relation to this