search

code-423n4 / 2022-02-aave-lens-findings

0 stars 0 forks source link

Gas Optimizations #44

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

LensHubStorage.CREATE_PROFILE_WITH_SIG_TYPEHASH is unused

    bytes32 internal constant CREATE_PROFILE_WITH_SIG_TYPEHASH =
        0x9ac3269d9abd6f8c5e850e07f21b199079e8a5cc4a55466d8c96ab0c4a5be403;
    // keccak256(
    // 'CreateProfileWithSig(string handle,string uri,address followModule,bytes followModuleData,uint256 nonce,uint256 deadline)'
    // );

https://github.com/code-423n4/2022-02-aave-lens/blob/aaf6c116345f3647e11a35010f28e3b90e7b4862/contracts/core/storage/LensHubStorage.sol#L8-L12

Duplicated code between getPowerByBlockNumber() and getDelegatedSupplyByBlockNumber()

Save gas by not duplicating approximate binary search algorthm in getPowerByBlockNumber() and getDelegatedSupplyByBlockNumber() in deployed code. Instead refactor and use a common internal function https://github.com/code-423n4/2022-02-aave-lens/blob/aaf6c116345f3647e11a35010f28e3b90e7b4862/contracts/core/FollowNFT.sol#L114-L144 https://github.com/code-423n4/2022-02-aave-lens/blob/aaf6c116345f3647e11a35010f28e3b90e7b4862/contracts/core/FollowNFT.sol#L156-L186

Duplicated code between _writeSnapshot() and _writeSupplySnapshot()

Save gas by not duplicating snapshot-for-block existence checks in _writeSnapshot() and _writeSupplySnapshot(). Instead refactor and use a common internal function, renaming the orignals to _writeSnapshotSnapshot() and _writeSnapshotSupplySnapshot() https://github.com/code-423n4/2022-02-aave-lens/blob/aaf6c116345f3647e11a35010f28e3b90e7b4862/contracts/core/FollowNFT.sol#L287-L296 https://github.com/code-423n4/2022-02-aave-lens/blob/aaf6c116345f3647e11a35010f28e3b90e7b4862/contracts/core/FollowNFT.sol#L304-L313

abi.encode() is less efficient than abi.encodePacked()

1.

return abi.encode(amount, currency, recipient, referralFee, endTimestamp);

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/collect/TimedFeeCollectModule.sol#L90

2.

return abi.encode(collectLimit, amount, currency, recipient, referralFee, endTimestamp);

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/collect/LimitedTimedFeeCollectModule.sol#L96

<array>.length should not be looked up in every loop of a for-loop

Even memory arrays incur the overhead of bit tests and bit shifts to calculate the array length 1.

for (uint256 i = 0; i < profileIds.length; ++i) {

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/InteractionLogic.sol#L47

2.

for (uint256 i = 0; i < byteHandle.length; ++i) {

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/PublishingLogic.sol#L403

3.

for (uint256 i = 0; i < vars.datas.length; ++i) {

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/LensHub.sol#L541

4.

for (uint256 i = 0; i < addresses.length; ++i) {

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/follow/ApprovalFollowModule.sol#L41

5.

for (uint256 i = 0; i < addresses.length; ++i) {

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/follow/ApprovalFollowModule.sol#L66

6.

for (uint256 i = 0; i < toCheck.length; ++i) {

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/follow/ApprovalFollowModule.sol#L128

++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.

for (uint256 i = 0; i < profileIds.length; ++i) {

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/InteractionLogic.sol#L47

2.

for (uint256 i = 0; i < byteHandle.length; ++i) {

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/PublishingLogic.sol#L403

3.

for (uint256 i = 0; i < vars.datas.length; ++i) {

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/LensHub.sol#L541

4.

for (uint256 i = 0; i < addresses.length; ++i) {

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/follow/ApprovalFollowModule.sol#L41

5.

for (uint256 i = 0; i < addresses.length; ++i) {

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/follow/ApprovalFollowModule.sol#L66

6.

for (uint256 i = 0; i < toCheck.length; ++i) {

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/follow/ApprovalFollowModule.sol#L128

It costs more gas to initialize variables to zero than to let the default of zero be applied

1.

for (uint256 i = 0; i < profileIds.length; ++i) {

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/InteractionLogic.sol#L47

2.

for (uint256 i = 0; i < byteHandle.length; ++i) {

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/PublishingLogic.sol#L403

3.

for (uint256 i = 0; i < vars.datas.length; ++i) {

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/LensHub.sol#L541

4.

for (uint256 i = 0; i < addresses.length; ++i) {

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/follow/ApprovalFollowModule.sol#L41

5.

for (uint256 i = 0; i < addresses.length; ++i) {

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/follow/ApprovalFollowModule.sol#L66

6.

for (uint256 i = 0; i < toCheck.length; ++i) {

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/follow/ApprovalFollowModule.sol#L128

7.

uint256 private lastInitializedRevision = 0;

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/upgradeability/VersionedInitializable.sol:29:

8.

uint256 lower = 0;

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/FollowNFT.sol:120:

9.

uint256 lower = 0;

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/FollowNFT.sol:162:

Storage of uints smaller than 32 bytes incur overhead

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 Use a larger size then downcast where needed

uint8 internal constant MAX_HANDLE_LENGTH = 31;

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/Constants.sol#L10

uint16 internal constant BPS_MAX = 10000;

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/FeeModuleBase.sol#L17

uint16 internal constant BPS_MAX = 10000;

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/ModuleGlobals.sol#L20

uint16 internal _treasuryFee;

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/ModuleGlobals.sol#L25

uint24 internal constant ONE_DAY = 24 hours;

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/collect/TimedFeeCollectModule.sol#L48

uint24 internal constant ONE_DAY = 24 hours;

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/collect/LimitedTimedFeeCollectModule.sol#L48

Using bools for storage incurs overhead

    // 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.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27

1.

mapping(address => bool) storage _followModuleWhitelisted

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/PublishingLogic.sol#L44

2.

mapping(address => bool) storage _followModuleWhitelisted

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/PublishingLogic.sol#L85

3.

mapping(address => bool) storage _collectModuleWhitelisted,

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/PublishingLogic.sol#L132

4.

mapping(address => bool) storage _referenceModuleWhitelisted

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/PublishingLogic.sol#L133

5.

mapping(address => bool) storage _collectModuleWhitelisted,

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/PublishingLogic.sol#L188

6.

mapping(address => bool) storage _referenceModuleWhitelisted

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/PublishingLogic.sol#L189

7.

mapping(address => bool) storage _referenceModuleWhitelisted

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/PublishingLogic.sol#L256

8.

mapping(address => bool) storage _collectModuleWhitelisted

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/PublishingLogic.sol#L306

9.

mapping(address => bool) storage _referenceModuleWhitelisted

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/PublishingLogic.sol#L325

10.

mapping(address => bool) storage _followModuleWhitelisted

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/PublishingLogic.sol#L346

11.

mapping(address => mapping(address => bool)) private _operatorApprovals;

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/base/ERC721Time.sol#L44

12.

bool private _initialized;

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/FollowNFT.sol#L45

13.

bool private _initialized;

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/CollectNFT.sol#L25

14.

mapping(address => bool) internal _profileCreatorWhitelisted;

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/storage/LensHubStorage.sol#L59

15.

mapping(address => bool) internal _followModuleWhitelisted;

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/storage/LensHubStorage.sol#L60

16.

mapping(address => bool) internal _collectModuleWhitelisted;

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/storage/LensHubStorage.sol#L61

17.

mapping(address => bool) internal _referenceModuleWhitelisted;

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/storage/LensHubStorage.sol#L62

18

mapping(address => bool) internal _currencyWhitelisted;

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/ModuleGlobals.sol#L22

Zer0dot commented 2 years ago

Great report, well documented and valid! We choose to keep the follow NFT algorithms inlined because we're not pressed for code size, so the added gas would not be worth it.

The uint padding is technically valid but constants and immutables are not stored in storage, so I don't believe this makes sense here. Furthermore, the treasury fee uint16 variable is packed in the same slot as the treasury, which reduces the cold SLOADs when modules fetch the treasury data. Variable zero initialization is handled by the optimizer afaik.

Unchecked is valid, and the typehash is a good catch! Changes overall are included here: https://github.com/aave/lens-protocol/pull/80