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: 7/21
Findings: 2
Award: $2,944.10
🌟 Selected for report: 1
🚀 Solo Findings: 0
Table of Contents:
@audit tagsThe code is annotated at multiple places with
//@auditcomments to pinpoint the issues. Please, pay attention to them for more details.
29: constructor(address hub) { 30: HUB = hub; //@audit 0 check 31: }
hubaddress hub should be address(0) checked.
This type of address(0) check is already done in the solution (even for hub), see in ModuleBase.sol:
File: ModuleBase.sol 23: constructor(address hub) { 24: if (hub == address(0)) revert Errors.InitParamsInvalid(); //@audit-ok address(0) check on hub 25: HUB = hub; 26: emit Events.ModuleBaseConstructed(hub, block.timestamp); 27: }
I suggest doing the same as in ModuleBase.sol.
34: function initialize( 35: uint256 profileId, 36: uint256 pubId, 37: string calldata name, 38: string calldata symbol 39: ) external override { //@audit no access controls / can be frontrun 40: if (_initialized) revert Errors.Initialized(); 41: _initialized = true; 42: _profileId = profileId; 43: _pubId = pubId; 44: super._initialize(name, symbol); 45: emit Events.CollectNFTInitialized(profileId, pubId, block.timestamp); 46: }
initialize can be called by everyone and front-runThe initialize function is missing access controls, allowing any user to initialize the contract. By front-running the contract deployers to initialize the contract, the incorrect parameters may be supplied, leaving the contract needing to be redeployed.
I recommend adding some type of access control (onlyHub? onlyGov?) to initialize().
48: constructor(address hub) { 49: HUB = hub; //@audit 0 check 50: }
hubSee Missing Address(0) check on hub for a similar explanation
53: function initialize( 54: uint256 profileId, 55: string calldata name, 56: string calldata symbol 57: ) external override { //@audit no access controls / can be frontrun 58: if (_initialized) revert Errors.Initialized(); 59: _initialized = true; 60: _profileId = profileId; 61: super._initialize(name, symbol); 62: emit Events.FollowNFTInitialized(profileId, block.timestamp); 63: }
initialize can be called by everyone and front-runSee initialize can be called by everyone and front-run for a similar explanation
57: constructor(address followNFTImpl, address collectNFTImpl) { 58: FOLLOW_NFT_IMPL = followNFTImpl; //@audit 0 check 59: COLLECT_NFT_IMPL = collectNFTImpl; //@audit 0 check 60: }
followNFTImplcollectNFTImplSee Missing Address(0) check on hub for a similar explanation
63: function initialize( 64: string calldata name, 65: string calldata symbol, 66: address newGovernance 67: ) external override initializer { //@audit no access controls / can be frontrun 68: super._initialize(name, symbol); 69: _setState(DataTypes.ProtocolState.Paused); 70: _setGovernance(newGovernance); 71: }
initialize can be called by everyone and front-runSee initialize can be called by everyone and front-run for a similar explanation
File: ERC721Enumerable.sol 124: function _addTokenToAllTokensEnumeration(uint256 tokenId) private { 125: _allTokensIndex[tokenId] = _allTokens.length; //@audit check for existence 126: _allTokens.push(tokenId); 127: }
This issue was already submitted as a medium issue. Mentioning it in the QA-Report still felt valuable. To avoid pushing multiple times a tokenId in the array and breaking the logic, _allTokensIndex[tokenId] should be checked for existence.
The comments say that a swap & pop should happen in this function.
138: // To prevent a gap in from's tokens array, we store the last token in the index of the token to delete, and 139: // then delete the last slot (swap and pop).
However, this isn't the case:
152: // This also deletes the contents at the last position of the array 153: delete _ownedTokensIndex[tokenId]; 154: delete _ownedTokens[from][lastTokenIndex];
I suggest either correcting the comment or doing like in _removeTokenFromAllTokensEnumeration():
177: // This also deletes the contents at the last position of the array 178: delete _allTokensIndex[tokenId]; 179: _allTokens.pop();
436: * @dev Internal function to invoke {IERC721Receiver-onERC721Received} on a target address.
should be
436: * @dev Private function to invoke {IERC721Receiver-onERC721Received} on a target address.
This would follow the practice from other places like ERC721Enumerable at L110, L121, L130 and L158:
110: * @dev Private function to add a token to this extension's ownership-tracking data structures. 121: * @dev Private function to add a token to this extension's token tracking data structures. 130: * @dev Private function to remove a token from this extension's ownership-tracking data structures. Note that 158: * @dev Private function to remove a token from this extension's token tracking data structures.
50: /** 51: * @notice This collect module levies a fee on collects and supports referrals. Thus, we need to decode data. 52: * //@audit missing @param profileId and @param pubId 53: * @param data The arbitrary data parameter, decoded into: 54: * uint256 collectLimit: The maximum amount of collects. 55: * uint256 amount: The currency total amount to levy. 56: * address currency: The currency address, must be internally whitelisted. 57: * address recipient: The custom recipient address to direct earnings to. 58: * uint16 referralFee: The referral fee to set. 59: * 60: * @return An abi encoded bytes parameter, which is the same as the passed data parameter. 61: */ 62: function initializePublicationCollectModule( 63: uint256 profileId, 64: uint256 pubId, 65: bytes calldata data 66: ) external override onlyHub returns (bytes memory) {
55: /** 56: * @notice This collect module levies a fee on collects and supports referrals. Thus, we need to decode data. 57: * //@audit missing @param profileId and @param pubId 58: * @param data The arbitrary data parameter, decoded into: 59: * uint256 collectLimit: The maximum amount of collects. 60: * uint256 amount: The currency total amount to levy. 61: * address currency: The currency address, must be internally whitelisted. 62: * address recipient: The custom recipient address to direct earnings to. 63: * uint16 referralFee: The referral fee to set. 64: * 65: * @return An abi encoded bytes parameter, containing (in order): collectLimit, amount, currency, recipient, referral fee & end timestamp. 66: */ 67: function initializePublicationCollectModule( 68: uint256 profileId, 69: uint256 pubId, 70: bytes calldata data 71: ) external override onlyHub returns (bytes memory) {
55: /** 56: * @notice This collect module levies a fee on collects and supports referrals. Thus, we need to decode data. 57: * //@audit missing @param profileId and @param pubId 58: * @param data The arbitrary data parameter, decoded into: 59: * uint256 amount: The currency total amount to levy. 60: * address currency: The currency address, must be internally whitelisted. 61: * address recipient: The custom recipient address to direct earnings to. 62: * uint16 referralFee: The referral fee to set. 63: * 64: * @return An abi encoded bytes parameter, containing (in order): amount, currency, recipient, referral fee & end timestamp. 65: */ 66: function initializePublicationCollectModule( 67: uint256 profileId, 68: uint256 pubId, 69: bytes calldata data 70: ) external override onlyHub returns (bytes memory) {
49: * @notice This follow module works on custom profile owner approvals. 50: * //@audit missing @param profileId 51: * @param data The arbitrary data parameter, decoded into: 52: * address[] addresses: The array of addresses to approve initially. 53: * 54: * @return An abi encoded bytes parameter, which is the same as the passed data parameter. 55: */ 56: function initializeFollowModule(uint256 profileId, bytes calldata data)
098: /** 099: * @notice Returns whether the given address is approved for the profile owned by a given address. 100: * 101: * @param profileOwner The profile owner of the profile to query the approval with. 102: * @param profileId The token ID of the profile to query approval with. 103: * @param toCheck The address to query approval for. 104: * 105: * @return //@audit incomplete @return definition 106: */ 107: function isApproved( 108: address profileOwner, 109: uint256 profileId, 110: address toCheck 111: ) external view returns (bool) {
115: /** 116: * @notice Returns whether the given addresses are approved for the profile owned by a given address. 117: * 118: * @param profileOwner The profile owner of the profile to query the approvals with. 119: * @param profileId The token ID of the profile to query approvals with. 120: * @param toCheck The address array to query approvals for. //@audit missing @return bool[] 121: */ 122: function isApprovedArray( 123: address profileOwner, 124: uint256 profileId, 125: address[] calldata toCheck 126: ) external view returns (bool[] memory) {
42: /** 43: * @notice This follow module levies a fee on follows. 44: * 45: * @param data The arbitrary data parameter, decoded into: //@audit missing @param profileId 46: * address currency: The currency address, must be internally whitelisted. 47: * uint256 amount: The currency total amount to levy. 48: * address recipient: The custom recipient address to direct earnings to. 49: * 50: * @return An abi encoded bytes parameter, which is the same as the passed data parameter. 51: */ 52: function initializeFollowModule(uint256 profileId, bytes calldata data) 53: external 54: override 55: onlyHub 56: returns (bytes memory)
12: /** 13: * @notice Initializes a follow module for a given Lens profile. This can only be called by the hub contract. 14: * 15: * @param profileId The token ID of the profile to initialize this follow module for. 16: * @param data Arbitrary data passed by the profile creator. //@audit missing @return bytes 17: */ 18: function initializeFollowModule(uint256 profileId, bytes calldata data) 19: external 20: returns (bytes memory);
It should be: @return An abi encoded bytes parameter, which is the same as the passed data parameter..
56: /** 57: * @notice Returns the governance power for a given user at a specified block number. 58: * 59: * @param user The user to query governance power for. 60: * @param blockNumber The block number to query the user's governance power at. //@audit missing @return uint256 61: */ 62: function getPowerByBlockNumber(address user, uint256 blockNumber) external returns (uint256);
64: /** 65: * @notice Returns the total delegated supply at a specified block number. This is the sum of all 66: * current available voting power at a given block. 67: * 68: * @param blockNumber The block number to query the delegated supply at. //@audit missing @return uint256 69: */ 70: function getDelegatedSupplyByBlockNumber(uint256 blockNumber) external returns (uint256);
449: /** 450: * @notice Returns the full profile struct associated with a given profile token ID. 451: * 452: * @param profileId The token ID of the profile to query. //@audit missing @return 453: */ 454: function getProfile(uint256 profileId) external view returns (DataTypes.ProfileStruct memory);
116: /** 117: * @dev Emitted when a profile is created. 118: * 119: * @param profileId The newly created profile's token ID. 120: * @param creator The profile creator, who created the token with the given profile ID. 121: * @param to The address receiving the profile with the given profile ID. 122: * @param handle The handle set for the profile. 123: * @param imageURI The image uri set for the profile. 124: * @param followModule The profile's newly set follow module. This CAN be the zero address. 125: * @param followModuleReturnData The data returned from the follow module's initialization. This is abi encoded 126: * and totally depends on the follow module chosen. //@audit missing @param followNFTURI 127: * @param timestamp The current block timestamp. 128: */ 129: event ProfileCreated( 130: uint256 indexed profileId, 131: address indexed creator, 132: address indexed to, 133: string handle, 134: string imageURI, 135: address followModule, 136: bytes followModuleReturnData, 137: string followNFTURI, 138: uint256 timestamp 139: );
28: /** 29: * @notice Follows the given profiles, executing the necessary logic and module calls before minting the follow 30: * NFT(s) to the follower. 31: * 32: * @param follower The address executing the follow. 33: * @param profileIds The array of profile token IDs to follow. 34: * @param followModuleDatas The array of follow module data parameters to pass to each profile's follow module. 35: * @param followNFTImpl The address of the follow NFT implementation, which has to be passed because it's an immutable in the hub. 36: * @param _profileById A pointer to the storage mapping of profile structs by profile ID. //@audit missing @param _profileIdByHandleHash 37: */ 38: function follow( 39: address follower, 40: uint256[] calldata profileIds, 41: bytes[] calldata followModuleDatas, 42: address followNFTImpl, 43: mapping(uint256 => DataTypes.ProfileStruct) storage _profileById, 44: mapping(bytes32 => uint256) storage _profileIdByHandleHash 45: ) external {
#0 - Zer0dot
2022-03-24T19:30:10Z
Again another great report from Dravee!
To the judge-- it's possible I may have previously denied the validity of the zero address checks, note that I now believe there's virtually no reason not to implement them, so they are valid.
However, the init frontrun issue is invalid because collect & follow NFTs are initialized in the same transaction as their cloning from the hub.
The ERC721Enumerable changes I'm not 100% sure on, this is basically CC'd from OpenZeppelin, I'll have to ask for clarity on that @miguelmtzinf @donosonaumczuk @tabshaikh if you guys want to weigh in.
Everything's updated here: https://github.com/aave/lens-protocol/pull/80
#1 - donosonaumczuk
2022-03-28T15:05:55Z
I'm not following Dravee on the ERC721Enumerable stuff.
_allTokensIndex[tokenId]should be checked for existence to avoid pushing multiple times a tokenId in the array
This is not necessary, as _addTokenToAllTokensEnumeration is called only from _beforeTokenTransfer when from == address(0), basically only on mints. So the token won't exist before.
_removeTokenFromOwnerEnumerationmissingpop()
Both _ownedTokensIndex and _ownedTokens are mappings, so they are just deleting the value with the delete. In the other function OpenZeppelin devs calls the pop() over _allTokens but because that is an array and they are not only deleting the value but also decreasing the length.
#2 - Zer0dot
2022-03-28T15:08:08Z
Nicely done @donosonaumczuk thanks for the input! We won't change anything in the ERC721 implementations forked from OZ.
#3 - 0xleastwood
2022-05-05T22:26:45Z
Awesome writeup, most of these are completely valid. I think the issue outlined in #53 and mentioned here does not point to an issue that may occur. Additionally, the frontrun issue is really only applicable to non-proxy setups which do not initialize and deploy in the same transaction. Fortunately, proxies do this safely.
Table of Contents:
@audit tagsThe code is annotated at multiple places with
//@auditcomments to pinpoint the issues. Please, pay attention to them for more details.
116: if (snapshotCount == 0) { 117: return 0; // Returning zero since this means the user never delegated and has no power 118: } 119: 120: uint256 lower = 0; 121: uint256 upper = snapshotCount - 1; //@audit uncheck (see condition L116)
As it's impossible for line 121 to underflow (see condition line 116), it should be wrapped inside an unchecked block.
158: if (snapshotCount == 0) { 159: return 0; // Returning zero since this means a delegation has never occurred 160: } 161: 162: uint256 lower = 0; 163: uint256 upper = snapshotCount - 1; //@audit uncheck (see condition line 158)
As it's impossible for line 163 to underflow (see condition line 158), it should be wrapped inside an unchecked block.
The modifier onlyWhitelistedProfileCreator is used only once:
File: LensHub.sol 142: function createProfile(DataTypes.CreateProfileData calldata vars) 143: external 144: override 145: whenNotPaused 146: onlyWhitelistedProfileCreator 147: {
Therefore, it can get inlined in createProfile to save gas
95: function setState(DataTypes.ProtocolState newState) external override { 96: if (msg.sender != _governance && msg.sender != _emergencyAdmin) //@audit gas: this is a sad path with always 2 SLOADs. There's a happy path. 97: revert Errors.NotGovernanceOrEmergencyAdmin(); 98: _setState(newState); 99: }
The current implementation of function setState favors a sad path which will always cost 2 SLOADs to check the condition.
It's possible to short-circuit this condition to provide a happy path which may cost only 1 SLOAD instead.
I suggest rewriting the function to:
function setState(DataTypes.ProtocolState newState) external override { if (msg.sender == _governance || msg.sender == _emergencyAdmin) { //@audit-info happy path. Use as 1st condition the most frequent one (here, we assume _governance calls this method the most often) _setState(newState); } else { revert Errors.NotGovernanceOrEmergencyAdmin(); } }
Another alternative:
function setState(DataTypes.ProtocolState newState) external override { if (msg.sender == _governance || msg.sender == _emergencyAdmin) { _setState(newState); return; } revert Errors.NotGovernanceOrEmergencyAdmin(); }
794: function getProfileIdByHandle(string calldata handle) external view override returns (uint256) { 795: bytes32 handleHash = keccak256(bytes(handle)); //@audit gas: var used only once, inline it 796: return _profileIdByHandleHash[handleHash]; 797: }
I suggest inlining handleHash L796.
856: function _createPost( 857: uint256 profileId, 858: string memory contentURI, //@audit gas: should be calldata (calls L333 and L374 are passing a calldata variable) 859: address collectModule, 860: bytes memory collectModuleData, //@audit gas: should be calldata (calls L335 and L376 are passing a calldata variable) 861: address referenceModule, 862: bytes memory referenceModuleData //@audit gas: should be calldata (calls L337 and L378 are passing a calldata variable) 863: ) internal {
calldata instead of memory for string contentURIcalldata instead of memory for bytes collectModuleDatacalldata instead of memory for bytes referenceModuleDataFor the 3 memory variables declared in _createPost(), the parent functions are actually passing a calldata variable:
329: function post(DataTypes.PostData calldata vars) external override whenPublishingEnabled { //@audit-info vars is using calldata 330: _validateCallerIsProfileOwnerOrDispatcher(vars.profileId); 331: _createPost( 332: vars.profileId, 333: vars.contentURI, //@audit-info calldata 334: vars.collectModule, 335: vars.collectModuleData, //@audit-info calldata 336: vars.referenceModule, 337: vars.referenceModuleData //@audit-info calldata 338: ); 339: } ... 342: function postWithSig(DataTypes.PostWithSigData calldata vars) //@audit-info vars is using calldata 343: external 344: override 345: whenPublishingEnabled 346: { ... 372: _createPost( 373: vars.profileId, 374: vars.contentURI, //@audit-info calldata 375: vars.collectModule, 376: vars.collectModuleData, //@audit-info calldata 377: vars.referenceModule, 378: vars.referenceModuleData //@audit-info calldata 379: );
Therefore, the function declaration should use calldata instead of memory for these 3 parameters.
This optimization is similar to the one for the internal function _createMirrorin LensNFTBase.sol which uses bytes calldata referenceModuleData:
File: LensHub.sol 890: function _createMirror( 891: uint256 profileId, 892: uint256 profileIdPointed, 893: uint256 pubIdPointed, 894: address referenceModule, 895: bytes calldata referenceModuleData //@audit-ok : calldata on internal because parent function passes a calldata variable 896: ) internal {
919: function _setFollowNFTURI(uint256 profileId, string memory followNFTURI) internal { //@audit gas: should be calldata (calls L255 and L285 are passing a calldata variable) 920: _profileById[profileId].followNFTURI = followNFTURI; 921: emit Events.FollowNFTURISet(profileId, followNFTURI, block.timestamp); 922: }
calldata instead of memory for string followNFTURIThe parent functions are actually passing a calldata variable:
289: function setFollowNFTURI(uint256 profileId, string calldata followNFTURI) //@audit-info calldata 290: external 291: override 292: whenNotPaused 293: { 294: _validateCallerIsProfileOwnerOrDispatcher(profileId); 295: _setFollowNFTURI(profileId, followNFTURI); //@audit-info calldata 296: } ... 299: function setFollowNFTURIWithSig(DataTypes.SetFollowNFTURIWithSigData calldata vars) //@audit-info calldata 300: external 301: override 302: whenNotPaused 303: { ... 325: _setFollowNFTURI(vars.profileId, vars.followNFTURI); //@audit-info calldata 326: }
Therefore, the function declaration should use calldata instead of memory for string followNFTURI
This optimization is similar to the one for the internal function _createMirrorin LensNFTBase.sol which uses bytes calldata referenceModuleData.
940: function _validateCallerIsProfileOwnerOrDispatcher(uint256 profileId) internal view { 941: if (msg.sender != ownerOf(profileId) && msg.sender != _dispatcherByProfile[profileId]) //@audit gas: this is a sad path with 2 SLOADs. A happy path is possible 942: revert Errors.NotProfileOwnerOrDispatcher(); 943: }
The current implementation of function _validateCallerIsProfileOwnerOrDispatcher favors a sad path which will always cost 2 SLOADs to check the condition.
It's possible to short-circuit this condition to provide a happy path which may cost only 1 SLOAD instead.
I suggest rewriting the function to:
function _validateCallerIsProfileOwnerOrDispatcher(uint256 profileId) internal view { if (msg.sender == ownerOf(profileId) || msg.sender == _dispatcherByProfile[profileId]) { return; } revert Errors.NotProfileOwnerOrDispatcher(); }
159: function _validateRecoveredAddress( 160: bytes32 digest, 161: address expectedAddress, 162: DataTypes.EIP712Signature memory sig //@audit gas: should be calldata (calls L78, L111 and L152 are passing a calldata variable) 163: ) internal view {
calldata instead of memoryThe calls to the internal function _validateRecoveredAddress pass a calldata variable:
51: function permit( 52: address spender, 53: uint256 tokenId, 54: DataTypes.EIP712Signature calldata sig //@audit-info calldata 55: ) external override { ... 78: _validateRecoveredAddress(digest, owner, sig); //@audit-info calldata ... 83: function permitForAll( 84: address owner, 85: address operator, 86: bool approved, 87: DataTypes.EIP712Signature calldata sig //@audit-info calldata 88: ) external override { ... 111: _validateRecoveredAddress(digest, owner, sig); //@audit-info calldata ... 127: function burnWithSig(uint256 tokenId, DataTypes.EIP712Signature calldata sig) //@audit-info calldata ... 152: _validateRecoveredAddress(digest, owner, sig); //@audit-info calldata
Therefore, the function declaration should use calldata instead of memory
This optimization is similar to the one for the internal function _createMirrorin LensNFTBase.sol which uses bytes calldata referenceModuleData.
342: function _initFollowModule( 343: uint256 profileId, 344: address followModule, 345: bytes memory followModuleData, //@audit gas: should be calldata (calls L64 and L95 are passing a calldata variable) 346: mapping(address => bool) storage _followModuleWhitelisted 347: ) private returns (bytes memory) {
calldata instead of memoryThe calls to the private function _initFollowModule pass a calldata variable:
39: function createProfile( 40: DataTypes.CreateProfileData calldata vars, //@audit-info calldata ... 61: bytes memory followModuleReturnData = _initFollowModule( 62: profileId, 63: vars.followModule, 64: vars.followModuleData, //@audit-info calldata 65: _followModuleWhitelisted 66: ); ... 80: function setFollowModule( 81: uint256 profileId, 82: address followModule, 83: bytes calldata followModuleData, //@audit-info calldata ... 92: bytes memory followModuleReturnData = _initFollowModule( 93: profileId, 94: followModule, 95: followModuleData, //@audit-info calldata 96: _followModuleWhitelisted 97: );
Therefore, the function declaration should use calldata instead of memory
This optimization is similar to the one for the internal function _createMirrorin LensNFTBase.sol which uses bytes calldata referenceModuleData.
If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.
As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {
Instances include:
core\modules\follow\ApprovalFollowModule.sol:41: for (uint256 i = 0; i < addresses.length; ++i) { core\modules\follow\ApprovalFollowModule.sol:66: for (uint256 i = 0; i < addresses.length; ++i) { core\modules\follow\ApprovalFollowModule.sol:128: for (uint256 i = 0; i < toCheck.length; ++i) { core\FollowNFT.sol:120: uint256 lower = 0; core\FollowNFT.sol:162: uint256 lower = 0; core\LensHub.sol:541: for (uint256 i = 0; i < vars.datas.length; ++i) { libraries\InteractionLogic.sol:47: for (uint256 i = 0; i < profileIds.length; ++i) { libraries\PublishingLogic.sol:403: for (uint256 i = 0; i < byteHandle.length; ++i) { upgradeability\VersionedInitializable.sol:29: uint256 private lastInitializedRevision = 0;
I suggest removing explicit initializations for default values.
As the solution employs pre-increments for all of its for-loops, I'm sure the sponsor is aware of the fact that pre-increments cost less gas compared to post-increments (about 5 gas)
However, some places outside loops were missed.
Instances include:
core\modules\collect\LimitedFeeCollectModule.sol:112: _dataByPublicationByProfile[profileId][pubId].currentCollects++; core\modules\collect\LimitedTimedFeeCollectModule.sol:123: _dataByPublicationByProfile[profileId][pubId].currentCollects++; core\LensHub.sol:887: _profileById[vars.profileId].pubCount++;
I suggest only replacing these, as some other places in the solution are actually using post-increments the right way. The logic would break if those other places (not mentioned here) are changed too.
In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.
Instances include:
core\modules\follow\ApprovalFollowModule.sol:41: for (uint256 i = 0; i < addresses.length; ++i) { core\modules\follow\ApprovalFollowModule.sol:66: for (uint256 i = 0; i < addresses.length; ++i) { core\modules\follow\ApprovalFollowModule.sol:128: for (uint256 i = 0; i < toCheck.length; ++i) { core\LensHub.sol:541: for (uint256 i = 0; i < vars.datas.length; ++i) { libraries\InteractionLogic.sol:47: for (uint256 i = 0; i < profileIds.length; ++i) { libraries\PublishingLogic.sol:403: for (uint256 i = 0; i < byteHandle.length; ++i) {
The code would go from:
for (uint256 i; i < numIterations; ++i) { // ... }
to:
for (uint256 i; i < numIterations;) { // ... unchecked { ++i; } }
The risk of overflow is inexistant for a uint256 here.
Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.
Caching the array length in the stack saves around 3 gas per iteration.
Here, I suggest storing the array's length in a variable before the for-loop, and use it instead:
core\modules\follow\ApprovalFollowModule.sol:41: for (uint256 i = 0; i < addresses.length; ++i) { core\modules\follow\ApprovalFollowModule.sol:66: for (uint256 i = 0; i < addresses.length; ++i) { core\modules\follow\ApprovalFollowModule.sol:128: for (uint256 i = 0; i < toCheck.length; ++i) { core\LensHub.sol:541: for (uint256 i = 0; i < vars.datas.length; ++i) { libraries\InteractionLogic.sol:47: for (uint256 i = 0; i < profileIds.length; ++i) { libraries\PublishingLogic.sol:403: for (uint256 i = 0; i < byteHandle.length; ++i) {
A division by 2 can be calculated by shifting one to the right.
While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.
I suggest replacing / 2 with >> 1 here:
core\modules\ModuleGlobals.sol:109: if (newTreasuryFee >= BPS_MAX / 2) revert Errors.InitParamsInvalid(); core\FollowNFT.sol:134: uint256 center = upper - (upper - lower) / 2; core\FollowNFT.sol:176: uint256 center = upper - (upper - lower) / 2;
I'm quite certain that the sponsor is aware of this optimization, as the library Errors contains a lot of custom errors. Therefore, I won't explain it (suffice to say, they are cheaper than require statements + revert strings).
The finding here is that the contracts ERC721Enumerable and ERC721Time don't benefit from this practice:
core\base\ERC721Enumerable.sol: 53: require(index < ERC721Time.balanceOf(owner), 'ERC721Enumerable: owner index out of bounds'); 68: require( 69 index < ERC721Enumerable.totalSupply(), 70 'ERC721Enumerable: global index out of bounds' 71 ); core\base\ERC721Time.sol: 77: require(owner != address(0), 'ERC721: balance query for the zero address'); 86: require(owner != address(0), 'ERC721: owner query for nonexistent token'); 95: require(mintTimestamp != 0, 'ERC721: mint timestamp query for nonexistent token'); 109: require(_exists(tokenId), 'ERC721: token data query for nonexistent token'); 131: require(_exists(tokenId), 'ERC721Metadata: URI query for nonexistent token'); 152: require(to != owner, 'ERC721: approval to current owner'); 154: require( 155 _msgSender() == owner || isApprovedForAll(owner, _msgSender()), 156 'ERC721: approve caller is not owner nor approved for all' 157 ); 166: require(_exists(tokenId), 'ERC721: approved query for nonexistent token'); 175: require(operator != _msgSender(), 'ERC721: approve to caller'); 202: require( 203 _isApprovedOrOwner(_msgSender(), tokenId), 204 'ERC721: transfer caller is not owner nor approved' 205 ); 230: require( 231 _isApprovedOrOwner(_msgSender(), tokenId), 232 'ERC721: transfer caller is not owner nor approved' 233 ); 262: require( 263 _checkOnERC721Received(from, to, tokenId, _data), 264 'ERC721: transfer to non ERC721Receiver implementer' 265 ); 293: require(_exists(tokenId), 'ERC721: operator query for nonexistent token'); 324: require( 325 _checkOnERC721Received(address(0), to, tokenId, _data), 326 'ERC721: transfer to non ERC721Receiver implementer' 327 ); 343: require(to != address(0), 'ERC721: mint to the zero address'); 344: require(!_exists(tokenId), 'ERC721: token already minted'); 395: require(ERC721Time.ownerOf(tokenId) == from, 'ERC721: transfer of token that is not own'); 396: require(to != address(0), 'ERC721: transfer to the zero address');
I suggest using custom errors in ERC721Enumerable and ERC721Time.
#0 - Zer0dot
2022-03-22T17:29:32Z
Okay this is possibly the best gas report I have ever seen. Huge props to Dravee!
#1 - Zer0dot
2022-03-23T22:47:50Z
Implementing a whole lot of this in a new PR, here are some notes:
= 0 initialization is there for clarity and I believe it's handled by the optimizer.Will write more, I'm currently at the unchecked increment section, which is valid. Anyway, C4 give this gigachad a medal.
#2 - Zer0dot
2022-03-24T15:11:26Z
Included unchecked increments, and cached array lengths (where possible) too!
Unchecked increments: https://github.com/aave/lens-protocol/commit/37ab8cea46ec6cf1f1a20dd4f5d720c49da06dc2 Array length caching: https://github.com/aave/lens-protocol/commit/a698476590fb58eb7f698079714cba31e9d10a5e
The SHR sacrifices readability too much, though it's still valid. Lastly the custom errors I would say are not valid since that's just how ERC721 is built, we don't want to stray away from the standard, even in revert messages as much as possible.
Overall this is a fantastic report!
#3 - Zer0dot
2022-03-31T18:26:54Z
PSA: The happy path short-circuiting is only partly valid in that it saves a minor amount of gas (2-3 opcodes) since even the "sad path" is evaluated lazily, if the first condition evaluates to false, the second condition is not evaluated.