search

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

0 stars 0 forks source link

QA Report #60

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

QA Report

Table of Contents:

Foreword

File: CollectNFT.sol

constructor(address hub)

29:     constructor(address hub) {
30:         HUB = hub; //@audit 0 check
31:     }

Missing Address(0) check on hub

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

function initialize()

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-run

The 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().

File: FollowNFT.sol

constructor(address hub)

48:     constructor(address hub) {
49:         HUB = hub;  //@audit 0 check
50:     }

Missing Address(0) check on hub

See Missing Address(0) check on hub for a similar explanation

function initialize()

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-run

See initialize can be called by everyone and front-run for a similar explanation

File: LensHub.sol

constructor(address followNFTImpl, address collectNFTImpl)

57:     constructor(address followNFTImpl, address collectNFTImpl) {
58:         FOLLOW_NFT_IMPL = followNFTImpl; //@audit 0 check
59:         COLLECT_NFT_IMPL = collectNFTImpl; //@audit 0 check
60:     }

Missing Address(0) check on followNFTImpl

Missing Address(0) check on collectNFTImpl

See Missing Address(0) check on hub for a similar explanation

function initialize()

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-run

See initialize can be called by everyone and front-run for a similar explanation

File: ERC721Enumerable.sol

function _addTokenToAllTokensEnumeration(uint256 tokenId)

File: ERC721Enumerable.sol
124:     function _addTokenToAllTokensEnumeration(uint256 tokenId) private {
125:         _allTokensIndex[tokenId] = _allTokens.length; //@audit check for existence
126:         _allTokens.push(tokenId);
127:     }

_allTokensIndex[tokenId] should be checked for existence

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.

function _removeTokenFromOwnerEnumeration(address from, uint256 tokenId)

Missing pop()

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();

File: ERC721Time.sol

function _checkOnERC721Received() private

Wrong comment

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.

File: LimitedFeeCollectModule.sol

function initializePublicationCollectModule()

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) {

Missing comment: @param profileId

Missing comment: @param pubId

File: LimitedTimedFeeCollectModule.sol

function initializePublicationCollectModule()

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) {

Missing comment: @param profileId

Missing comment: @param pubId

File: TimedFeeCollectModule.sol

function initializePublicationCollectModule()

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) {

Missing comment: @param profileId

Missing comment: @param pubId

File: ApprovalFollowModule.sol

function initializeFollowModule()

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)

Missing comment: @param profileId

function isApproved()

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) {

Incomplete @return definition (no description)

function isApprovedArray()

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) {

Missing comment: @return bool[]

File: FeeFollowModule.sol

function initializeFollowModule()

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)

Missing comment: @param profileId

File: IFollowModule.sol

function initializeFollowModule()

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);

Missing comment: @return bytes

It should be: @return An abi encoded bytes parameter, which is the same as the passed data parameter..

File: IFollowNFT.sol

function getPowerByBlockNumber()

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);

Missing comment: @return uint256

function getDelegatedSupplyByBlockNumber()

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);

Missing comment: @return uint256

File: ILensHub.sol

function getProfile()

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);

Missing comment: @return DataTypes.ProfileStruct

File: Events.sol

event ProfileCreated()

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:     );

Missing comment: @param followNFTURI

File: InteractionLogic.sol

function follow()

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 {

Missing comment: @param _profileIdByHandleHash

Zer0dot commented 2 years ago

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

donosonaumczuk commented 2 years ago

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.

_removeTokenFromOwnerEnumeration missing pop()

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.

Zer0dot commented 2 years ago

Nicely done @donosonaumczuk thanks for the input! We won't change anything in the ERC721 implementations forked from OZ.

0xleastwood commented 2 years ago

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.