Open code423n4 opened 2 years ago
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.Actually on the treasury fee point, after some discussion, I think it may be valid to implement, linking issue #62 in relation to this
1. Loops can be more efficient
Impact
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.
Proof of Concept
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/LensHub.sol#L541
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/follow/ApprovalFollowModule.sol#L41
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/follow/ApprovalFollowModule.sol#L66
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/follow/ApprovalFollowModule.sol#L128
The same situation are in other scope contracts where loops use. Remix
Recommended Mitigation Steps
Remove explicit 0 initialization of for loop index variable.
2. Long Revert Strings
Impact
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.
Proof of Concept
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/base/ERC721Time.sol#L77
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/base/ERC721Time.sol#L86
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/base/ERC721Time.sol#L95
There are several other places throughout the codebase where the same optimization can be used.
Tools
https://planetcalc.com/9029/
Recommended Mitigation Steps
Shorten the revert strings to fit in 32 bytes.
3. > 0 can be replaced with != 0 for gas optimisation
Vulnerability details
Impact
!= 0 is a cheaper operation compared to > 0, when dealing with uint.
Proof of Concept
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
Tools
Remix
Recommended Mitigation Steps
4. Adding unchecked directive can save gas
Impact
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.
Proof of Concept
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/collect/LimitedTimedFeeCollectModule.sol#L72
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/collect/TimedFeeCollectModule.sol#L71
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/FollowNFT.sol#L106-L145
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/FollowNFT.sol#L148-L187
Tools
Remix
Recommended Mitigation Steps
Consider using 'unchecked' where it is safe to do so.
5. A variable is being assigned its default value
Vulnerability details
Impact
A variable is being assigned its default value which is unnecessary. Removing the assignment will save gas when deploying.
Proof of Concept
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/upgradeability/VersionedInitializable.sol#L29
Tools
Remix
Recommended Mitigation Steps
Remove the assignment.
6. Less than 256 uints are not gas efficient
Vulnerability details
Impact
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.
Proof of Concept
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
Tools
Remix
Recommended Mitigation Steps
Consider to review all uint types. Change them with uint256 If the integer is not necessary to present with uint24.
7. Change string to byteX if possible
Vulnerability details
Impact
In the Constants library, declaring the constants with type bytes32 can save gas.
Proof of Concept
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/libraries/Constants.sol#L6-L9
Tools
https://medium.com/layerx/how-to-reduce-gas-cost-in-solidity-f2e5321e0395#2a78
Recommended Mitigation Steps
8. Modifier
whenNotPaused
and function_validateNotPaused
Vulnerability details
Impact
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.Proof of Concept
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
Tools
Remix
Recommended Mitigation Steps
Remove function
_validateNotPaused,
place its logic directly in the modifier whenNotPaused`.9. Checking non-zero value can avoid an external call to save gas
Vulnerability details
Impact
Checking if
adjustedAmount
andtreasuryAmount > 0
before making the external library call to IERC20(currency) can save gas by avoiding the external call in such situations.Proof of Concept
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.
Tools
Remix
Recommended Mitigation Steps
10. Change encode to encodePacked
Vulnerability details
Impact
Change abi.encode to abi.encodePacked can save gas.
Proof of Concept
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.
Tools
Remix
Recommended Mitigation Steps
11. Use calldata instead of memory for external functions where the function argument is read-only
Vulnerability details
Impact
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.»
Proof of Concept
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/libraries/PublishingLogic.sol#L124
Tools
Remix
Recommended Mitigation Steps
Use calldata instead of memory for external functions where the function argument is read-only.
12. Changing bool to uint256 can save gas
Vulnerability details
Impact
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.
Proof of Concept
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/storage/LensHubStorage.sol#L59-L62
Tools
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol#L23-L27
Recommended Mitigation Steps
13. Internal functions can be private if the contract is not herited
Vulnerability details
Impact
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.
Proof of Concept
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
Tools
Remix
Recommended Mitigation Steps
Define these functions as private.
14. Caching variables
Impact
Some of the variable can be cached to slightly reduce gas usage.
Proof of Concept
HUB
can be cached. https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/FollowNFT.sol#L207-L212Tools
Remix
Recommended Mitigation Steps
Consider caching those variable for read and make sure write back to storage.