The code under review can be found in 2023-07-lens.
| ID | Description | Severity |
|---|---|---|
| M-01 | Token guardian protection doesn't account for approved operators in approve() |
Medium |
| M-02 | Inconsistent encoding of arrays in MetaTxLib |
Medium |
| M-03 | EIP-712 typehash is incorrect for several functions in MetaTxLib |
Medium |
| M-04 | Whitelisted profile creators could accidentally break migration for V1 profiles | Medium |
| M-05 | Users can unfollow through FollowNFT contract when LensHub is paused by governance |
Medium |
| M-06 | Users cannot unfollow if they do not own the FollowNFT of the followTokenId used for their profile |
Medium |
| M-07 | tryMigrate() doesn't ensure that followerProfileId isn't already following |
Medium |
| M-08 | Identifying publications using its ID makes the protocol vulnerable to blockchain re-orgs | Medium |
| L-01 | Avoid directly minting follow NFTs to the profile owner in processBlock() |
Low |
| L-02 | Regular approvals are not used in access controls in FollowNFT.sol |
Low |
| L-03 | Users have no way of revoking their signatures in LensHub |
Low |
| L-04 | Removing ERC721Enumerable functionality might break composability with other protocols |
Low |
| L-05 | Delegated executor configs are not cleared on transfer | Low |
| L-06 | act() in ActionLib.sol doesn't check if the target publication is a mirror |
Low |
| L-07 | LensHub contract cannot be unpaused if emergency admin and governance is the same address |
Low |
| L-08 | Only 255 action modules can ever be whitelisted | Low |
| L-09 | Setting transactionExecutor to msg.sender in createProfile() might limit functionality |
Low |
| L-10 | Un-migratable handles might exist in Lens Protocol V1 | Low |
| L-11 | Avoid minting handles that have a tokenId of 0 in mintHandle() |
Low |
| L-12 | Relayer can choose amount of gas when calling function in meta-transactions | Low |
| N-01 | approveFollow() should allow followerProfileId = 0 to cancel follow approvals |
Non-Critical |
| N-02 | Redundant check in _isApprovedOrOwner() can be removed |
Non-Critical |
| N-03 | whenNotPaused modifier is redundant for burn() in LensProfiles.sol |
Non-Critical |
| N-04 | unfollow() should be allowed for burnt profiles |
Non-Critical |
According to the README, when an address has token guardian enabled, approvals should not work for the tokens owned by that address:
Token Guardian: Protection mechanism for the tokens held by an address, which restricts transfers and approvals when enabled. See LIP-4 for more.
In LensHandles.sol, token guardian is enforced by the _hasTokenGuardianEnabled() check in the approve() function:
function approve(address to, uint256 tokenId) public override(IERC721, ERC721) {
// We allow removing approvals even if the wallet has the token guardian enabled
if (to != address(0) && _hasTokenGuardianEnabled(msg.sender)) {
revert HandlesErrors.GuardianEnabled();
}
super.approve(to, tokenId);
}However, this check is inadequate as approved operators (addresses approved using setApprovalForAll() by the owner) are also allowed to call approve(). We can see this in Openzeppelin's ERC-721 implementation:
require(
_msgSender() == owner || isApprovedForAll(owner, _msgSender()),
"ERC721: approve caller is not token owner or approved for all"
);As such, even if an owner has token guardian enabled, approvals can still be set for his tokens by other approved operators, leaving the owner's tokens vulnerable. For example:
- Alice sets Bob as an approved operator using
setApprovalForAll(). - Alice enables token guardian using
enableTokenGuardian(). - If Bob wants to set approvals for Alice's tokens, he can do so by:
- Disabling his own token guardian using
DANGER__disableTokenGuardian(). - Calling
approve()for Alice's tokens. This will still work, even though Alice has token guardian enabled.
- Disabling his own token guardian using
Note that the approve() function in LensProfiles.sol also has the same vulnerability.
As token guardian protection in approve() does not account for approved operators, although an owner has token guardian enabled, approved operators will still be able to set approvals for his tokens.
The following Foundry test demonstrates the example above:
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;
import 'forge-std/Test.sol';
import '../contracts/namespaces/LensHandles.sol';
contract TokenGuardian_POC is Test {
LensHandles lensHandles;
address ALICE;
address BOB;
uint256 tokenId;
function setUp() public {
// Setup LensHandles contract
lensHandles = new LensHandles(address(this), address(0), 0);
// Setup Alice and Bob addresses
ALICE = makeAddr("Alice");
BOB = makeAddr("Bob");
// Mint "alice.lens" to Alice
tokenId = lensHandles.mintHandle(ALICE, "alice");
}
function testCanApproveWhileTokenGuardianEnabled() public {
// Alice disables tokenGuardian to set Bob as an approved operator
vm.startPrank(ALICE);
lensHandles.DANGER__disableTokenGuardian();
lensHandles.setApprovalForAll(BOB, true);
// Alice re-enables tokenGuardian
lensHandles.enableTokenGuardian();
vm.stopPrank();
// Bob disables tokenGuardian for himself
vm.startPrank(BOB);
lensHandles.DANGER__disableTokenGuardian();
// Alice still has tokenGuardian enabled
assertEq(lensHandles.getTokenGuardianDisablingTimestamp(ALICE), 0);
// However, Bob can still set approvals for Alice's handle
lensHandles.approve(address(0x1337), tokenId);
vm.stopPrank();
assertEq(lensHandles.getApproved(tokenId), address(0x1337));
}
}Consider checking if the token's owner has token guardian enabled as well:
function approve(address to, uint256 tokenId) public override(IERC721, ERC721) {
// We allow removing approvals even if the wallet has the token guardian enabled
- if (to != address(0) && _hasTokenGuardianEnabled(msg.sender)) {
+ if (to != address(0) && (_hasTokenGuardianEnabled(msg.sender) || _hasTokenGuardianEnabled(_ownerOf(tokenId)))) {
revert HandlesErrors.GuardianEnabled();
}
super.approve(to, tokenId);
}According to the EIP-712 specification, arrays are encoded by concatenating its elements and passing the result to keccak256:
The array values are encoded as the
keccak256hash of the concatenated encodeData of their contents (i.e. the encoding ofSomeType[5]is identical to that of a struct containing five members of typeSomeType).
An example of a correct implementation can be seen in validateUnfollowSignature(), where the idsOfProfilesToUnfollow array is passed to keccak256 after using abi.encodePacked():
keccak256(abi.encodePacked(idsOfProfilesToUnfollow)),However, some other functions in MetaTxLib encode arrays differently, which differs from the EIP-712 specification.
Some functions do not encode the array at all, and pass the array to abi.encode() alongside other arguments in its struct. For example, in validatePostSignature(), the postParams.actionModules array is not encoded by itself:
abi.encode(
Typehash.POST,
postParams.profileId,
keccak256(bytes(postParams.contentURI)),
postParams.actionModules,
_hashActionModulesInitDatas(postParams.actionModulesInitDatas),
postParams.referenceModule,
keccak256(postParams.referenceModuleInitData),
_getAndIncrementNonce(signature.signer),
signature.deadline
)Other instances of this include:
mirrorParams.referrerProfileIdsandmirrorParams.referrerPubIdsinvalidateMirrorSignature()publicationActionParams.referrerProfileIdsandpublicationActionParams.referrerPubIdsinvalidateActSignature()- All functions that use
_abiEncode(), namelyvalidateCommentSignature()andvalidateQuoteSignature()
Secondly, the validateChangeDelegatedExecutorsConfigSignature() function encodes the delegatedExecutors and approvals arrays using abi.encodePacked(), but do not pass it to keccak256:
abi.encode(
Typehash.CHANGE_DELEGATED_EXECUTORS_CONFIG,
delegatorProfileId,
abi.encodePacked(delegatedExecutors),
abi.encodePacked(approvals),
configNumber,
switchToGivenConfig,
nonce,
deadline
)As arrays are encoded incorrectly, the signature verification in the functions listed above is not EIP-712 compliant.
Contracts or dapps/backends that encode arrays according to the rules specified in EIP-712 will end up with different signatures, causing any of the functions listed above to revert when called.
Moreover, the inconsistent encoding of arrays might be extremely confusing to developers who wish to use these functions to implement meta-transactions.
Consider encoding arrays correctly in the functions listed above, which can be achieved by calling abi.encodePacked() on the array and passing its results to keccak256.
In LensHub.sol, the second parameter of setProfileMetadataURIWithSig() is declared as metadataURI:
function setProfileMetadataURIWithSig(
uint256 profileId,
string calldata metadataURI,
Types.EIP712Signature calldata signature
) external override whenNotPaused onlyProfileOwnerOrDelegatedExecutor(signature.signer, profileId) {However, its EIP-712 typehash stores the parameter as metadata instead:
bytes32 constant SET_PROFILE_METADATA_URI = keccak256('SetProfileMetadataURI(uint256 profileId,string metadata,uint256 nonce,uint256 deadline)');The PostParams struct (which is used for postWithSig()) has address[] actionModules and bytes[] actionModulesInitDatas as its third and fourth fields:
struct PostParams {
uint256 profileId;
string contentURI;
address[] actionModules;
bytes[] actionModulesInitDatas;
address referenceModule;
bytes referenceModuleInitData;
}However, the third and fourth fields in its typehash are declared as address collectModule and bytes collectModuleInitData instead:
bytes32 constant POST = keccak256('Post(uint256 profileId,string contentURI,address collectModule,bytes collectModuleInitData,address referenceModule,bytes referenceModuleInitData,uint256 nonce,uint256 deadline)');This occurs for the commentWithSig() and quoteWithSig() functions as well:
bytes32 constant QUOTE = keccak256('Quote(uint256 profileId,string contentURI,uint256 pointedProfileId,uint256 pointedPubId,uint256[] referrerProfileIds,uint256[] referrerPubIds,bytes referenceModuleData,address collectModule,bytes collectModuleInitData,address referenceModule,bytes referenceModuleInitData,uint256 nonce,uint256 deadline)');bytes32 constant COMMENT = keccak256('Comment(uint256 profileId,string contentURI,uint256 pointedProfileId,uint256 pointedPubId,uint256[] referrerProfileIds,uint256[] referrerPubIds,bytes referenceModuleData,address collectModule,bytes collectModuleInitData,address referenceModule,bytes referenceModuleInitData,uint256 nonce,uint256 deadline)');The fourth and fifth fields in the MirrorParams struct (which is used for mirrorWithSig()) are declared as referrerProfileIds and referrerPubIds:
struct MirrorParams {
uint256 profileId;
uint256 pointedProfileId;
uint256 pointedPubId;
uint256[] referrerProfileIds;
uint256[] referrerPubIds;
bytes referenceModuleData;
}However, its EIP-712 typehash declares these fields as referrerProfileId and referrerPubId instead:
bytes32 constant MIRROR = keccak256('Mirror(uint256 profileId,uint256 pointedProfileId,uint256 pointedPubId,uint256[] referrerProfileId,uint256[] referrerPubId,bytes referenceModuleData,uint256 nonce,uint256 deadline)');Due to the use of incorrect typehashes, the signature verification in the functions listed above is not EIP-712 compliant.
Contracts or dapps/backends that use "correct" typehashes that match the parameters of these functions will end up generating different signatures, causing them to revert when called.
Amend the typehashes shown above to have matching parameters with their respective functions.
Profiles that exist before the V2 upgrade are migrated using the batchMigrateProfiles() function, which works by minting the profile's handle and linking it to their profile:
string memory handle = StorageLib.getProfile(profileId).__DEPRECATED__handle;
if (bytes(handle).length == 0) {
return; // Already migrated
}
bytes32 handleHash = keccak256(bytes(handle));
// We check if the profile is the "lensprotocol" profile by checking profileId != 1.
// "lensprotocol" is the only edge case without the .lens suffix:
if (profileId != LENS_PROTOCOL_PROFILE_ID) {
assembly {
let handle_length := mload(handle)
mstore(handle, sub(handle_length, DOT_LENS_SUFFIX_LENGTH)) // Cut 5 chars (.lens) from the end
}
}
// We mint a new handle on the LensHandles contract. The resulting handle NFT is sent to the profile owner.
uint256 handleId = lensHandles.migrateHandle(profileOwner, handle);
// We link it to the profile in the TokenHandleRegistry contract.
tokenHandleRegistry.migrationLink(handleId, profileId);For example, a profile with the handle "alice.lens" will receive a "alice" LensHandles NFT post-migration.
However, whitelisted profile creators are able to mint any handle using mintHandle() in the LensHandles contract. This makes it possible for any whitelisted profile creator to mint a handle corresponding to a V1 profile before the profile is migrated.
If this occurs, batchMigrateProfiles() will always revert for the corresponding V1 profile as the same handle cannot be minted twice, thereby breaking migration for that profile.
If a whitelisted profile creator accidentally mints a handle that already belongs to a V1 profile, that profile cannot be migrated.
The Foundry test below demonstrates how batchMigrateProfiles() will revert if a V1 profile's handle has already been minted. It can be run with the following command:
forge test --match-test testProfileCreatorCanBreakProfileMigration -vvv
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;
import 'test/base/BaseTest.t.sol';
contract ProfileMigration_POC is BaseTest {
LensHubHelper hubProxy;
function setUp() public override {
super.setUp();
// Add toLegacyV1Profile() function to LensHub
LensHubHelper implementation = new LensHubHelper({
lensHandlesAddress: address(lensHandles),
tokenHandleRegistryAddress: address(tokenHandleRegistry),
tokenGuardianCooldown: PROFILE_GUARDIAN_COOLDOWN
});
vm.prank(deployer);
hubAsProxy.upgradeTo(address(implementation));
// Cast proxy to LensHubHelper interface
hubProxy = LensHubHelper(address(hubAsProxy));
}
function testProfileCreatorCanBreakProfileMigration() public {
// Create a v1 profile with the handle "alice.lens"
uint256 profileId = _createProfile(address(this));
hubProxy.toLegacyV1Profile(profileId, "alice.lens");
// Whitelisted profile creator accidentally mints a "alice.lens" handle
vm.prank(lensHandles.OWNER());
lensHandles.mintHandle(address(this), "alice");
// V1 profile will revert when migrated as the handle already exists
vm.expectRevert("ERC721: token already minted");
hubProxy.batchMigrateProfiles(_toUint256Array(profileId));
}
}
contract LensHubHelper is LensHub {
constructor(
address lensHandlesAddress,
address tokenHandleRegistryAddress,
uint256 tokenGuardianCooldown
) LensHub(
address(0),
address(0),
address(0),
lensHandlesAddress,
tokenHandleRegistryAddress,
address(0),
address(0),
address(0),
tokenGuardianCooldown
) {}
function toLegacyV1Profile(uint256 profileId, string memory handle) external {
Types.Profile storage profile = StorageLib.getProfile(profileId);
profile.__DEPRECATED__handle = handle;
delete profile.metadataURI;
}
}Ensure that the handle of a V1 profile cannot be minted through mintHandle(). This validation will probably have to be done off-chain, as it is unfeasible to check all existing handles on-chain with a reasonable gas cost.
When the LensHub contract has been paused by governance (_state set to ProtocolState.Paused), users should not be able unfollow profiles. This can be inferred as the unfollow() function has the whenNotPaused modifier:
function unfollow(uint256 unfollowerProfileId, uint256[] calldata idsOfProfilesToUnfollow)
external
override
whenNotPausedHowever, in the FollowNFT contract, which is deployed for each profile that has followers, the removeFollower() and burn() functions do not check if the LensHub contract is paused:
function removeFollower(uint256 followTokenId) external override {
address followTokenOwner = ownerOf(followTokenId);
if (followTokenOwner == msg.sender || isApprovedForAll(followTokenOwner, msg.sender)) {
_unfollowIfHasFollower(followTokenId);
} else {
revert DoesNotHavePermissions();
}
} function burn(uint256 followTokenId) public override {
_unfollowIfHasFollower(followTokenId);
super.burn(followTokenId);
}As such, whenever the system has been paused by governance, users will still be able to unfollow profiles by wrapping their followNFT and then calling either removeFollower() or burn().
Users are able to unfollow profiles when the system is paused, which they should not be able to do.
This could be problematic if governance ever needs to temporarily pause unfollow functionality (eg. for a future upgrade, or unfollowing functionality has a bug, etc...).
The Foundry test below demonstrates how users will still be able to unfollow profiles by calling wrap() and removeFollower(), even after the system has been paused by governance. It can be run with the following command:
forge test --match-test testCanUnfollowWhilePaused -vvv
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;
import 'test/base/BaseTest.t.sol';
contract Unfollow_POC is BaseTest {
address targetProfileOwner;
uint256 targetProfileId;
FollowNFT targetFollowNFT;
address follower;
uint256 followerProfileId;
uint256 followTokenId;
function setUp() public override {
super.setUp();
// Create profile for target
targetProfileOwner = makeAddr("Target");
targetProfileId = _createProfile(targetProfileOwner);
// Create profile for follower
follower = makeAddr("Follower");
followerProfileId = _createProfile(follower);
// Follower follows target
vm.prank(follower);
followTokenId = hub.follow(
followerProfileId,
_toUint256Array(targetProfileId),
_toUint256Array(0),
_toBytesArray('')
)[0];
targetFollowNFT = FollowNFT(hub.getProfile(targetProfileId).followNFT);
}
function testCanUnfollowWhilePaused() public {
// Governance pauses system
vm.prank(governance);
hub.setState(Types.ProtocolState.Paused);
assertEq(uint8(hub.getState()), uint8(Types.ProtocolState.Paused));
// unfollow() reverts as system is paused
vm.startPrank(follower);
vm.expectRevert(Errors.Paused.selector);
hub.unfollow(followerProfileId, _toUint256Array(targetProfileId));
// However, follower can still unfollow through FollowNFT contract
targetFollowNFT.wrap(followTokenId);
targetFollowNFT.removeFollower(followTokenId);
vm.stopPrank();
// follower isn't following anymore
assertFalse(targetFollowNFT.isFollowing(followerProfileId));
}
}All FollowNFT contracts should check that the LensHub contract isn't paused before allowing removeFollower() or burn() to be called. This can be achieved by doing the following:
- Add a
whenNotPausedmodifier toFollowNFT.sol:
modifier whenNotPaused() {
if (ILensHub(HUB).getState() == Types.ProtocolState.Paused) {
revert Errors.Paused();
}
_;
}- Use the modifier on
removeFollower()andburn():
- function removeFollower(uint256 followTokenId) external override {
+ function removeFollower(uint256 followTokenId) external override whenNotPaused {
// Some code here...
}- function burn(uint256 followTokenId) public override {
+ function burn(uint256 followTokenId) public override whenNotPaused {
// Some code here...
}[M-06] Users cannot unfollow if they do not own the FollowNFT of the followTokenId used for their profile
If the followTokenId of a profile is wrapped, users will only be able to unfollow if they are either:
- The owner of the follow NFT.
- An approved operator of the follow NFT's owner.
This can be seen in the unfollow() function of FollowNFT.sol:
// Follow token is wrapped.
address unfollowerProfileOwner = IERC721(HUB).ownerOf(unfollowerProfileId);
// Follower profile owner or its approved delegated executor must hold the token or be approved-for-all.
if (
(followTokenOwner != unfollowerProfileOwner) &&
(followTokenOwner != transactionExecutor) &&
!isApprovedForAll(followTokenOwner, transactionExecutor) &&
!isApprovedForAll(followTokenOwner, unfollowerProfileOwner)
) {
revert DoesNotHavePermissions();
}As seen from above, users that are not the owner or do not have approval for the wrapped follow NFT will not be able to unfollow. This is problematic as users are able to follow with a followTokenId without owning the corresponding follow NFT.
For example, someone who holds a follow NFT can call approveFollow() for a user. The user can then call follow() with the corresponding followTokenId, which works as _followWithWrappedToken() checks for follow approval:
bool isFollowApproved = _followApprovalByFollowTokenId[followTokenId] == followerProfileId;
address followerProfileOwner = IERC721(HUB).ownerOf(followerProfileId);
if (
!isFollowApproved &&
followTokenOwner != followerProfileOwner &&
followTokenOwner != transactionExecutor &&
!isApprovedForAll(followTokenOwner, transactionExecutor) &&
!isApprovedForAll(followTokenOwner, followerProfileOwner)
) {
revert DoesNotHavePermissions();
}Now, if the user wants to unfollow, he will be unable to do so by himself, and is forced to rely on the follow NFT owner to unfollow for his profile.
Users that follow using a wrapped followTokenId that they do not own will not be unfollow the profile. This is incorrect as a profile owner should have full control over who the profile does/does not follow.
The Foundry test below demonstrates that unfollow() will revert when users do not own the FollowNFT, even when unfollowing with their own profile. It can be run with the following command:
forge test --match-test testCannotUnfollowWithoutFollowNFT -vvv
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;
import 'test/base/BaseTest.t.sol';
import 'contracts/interfaces/IFollowNFT.sol';
contract Unfollow_POC is BaseTest {
address targetProfileOwner;
address ALICE;
address BOB;
uint256 targetProfileId;
uint256 aliceProfileId;
uint256 bobProfileId;
function setUp() public override {
super.setUp();
// Setup addresses for target, Alice and Bob
targetProfileOwner = makeAddr("Target");
ALICE = makeAddr("Alice");
BOB = makeAddr("Bob");
// Create profile for target, Alice and Bob
targetProfileId = _createProfile(targetProfileOwner);
aliceProfileId = _createProfile(ALICE);
bobProfileId = _createProfile(BOB);
}
function testCannotUnfollowWithoutFollowNFT() public {
// Alice follows target
vm.startPrank(ALICE);
uint256 followTokenId = hub.follow(
aliceProfileId,
_toUint256Array(targetProfileId),
_toUint256Array(0),
_toBytesArray('')
)[0];
// Get followNFT contract created
FollowNFT followNFT = FollowNFT(hub.getProfile(targetProfileId).followNFT);
// Alice lets Bob follow using her followTokenId
followNFT.wrap(followTokenId);
followNFT.approveFollow(bobProfileId, followTokenId);
vm.stopPrank();
// Bob follows using her followTokenId
vm.startPrank(BOB);
hub.follow(
bobProfileId,
_toUint256Array(targetProfileId),
_toUint256Array(followTokenId),
_toBytesArray('')
);
assertTrue(followNFT.isFollowing(bobProfileId));
// After a while, Bob wants to unfollow.
// However, unfollow() reverts as he doesn't own the followNFT
vm.expectRevert(IFollowNFT.DoesNotHavePermissions.selector);
hub.unfollow(bobProfileId, _toUint256Array(targetProfileId));
vm.stopPrank();
}
}In unfollow(), consider allowing the owner of unfollowerProfileId to unfollow as well:
// Follow token is wrapped.
address unfollowerProfileOwner = IERC721(HUB).ownerOf(unfollowerProfileId);
// Follower profile owner or its approved delegated executor must hold the token or be approved-for-all.
if (
+ transactionExecutor != unfollowerProfileOwner &&
(followTokenOwner != unfollowerProfileOwner) &&
(followTokenOwner != transactionExecutor) &&
!isApprovedForAll(followTokenOwner, transactionExecutor) &&
!isApprovedForAll(followTokenOwner, unfollowerProfileOwner)
) {
revert DoesNotHavePermissions();
}In FollowNFT.sol, the tryMigrate() function is used to migrate users who were following before the V2 upgrade. It does so by updating _followTokenIdByFollowerProfileId and _followDataByFollowTokenId, which are state variables introduced in the V2 upgrade:
_followTokenIdByFollowerProfileId[followerProfileId] = followTokenId;
uint48 mintTimestamp = uint48(StorageLib.getTokenData(followTokenId).mintTimestamp);
_followDataByFollowTokenId[followTokenId].followerProfileId = uint160(followerProfileId);
_followDataByFollowTokenId[followTokenId].originalFollowTimestamp = mintTimestamp;
_followDataByFollowTokenId[followTokenId].followTimestamp = mintTimestamp;Since _followTokenIdByFollowerProfileId is a new state variable, it will be set to 0 for users who were following before the V2 upgrade. This allows old followers to call follow() to follow the profile again before tryMigrate() is called:
function follow(
uint256 followerProfileId,
address transactionExecutor,
uint256 followTokenId
) external override onlyHub returns (uint256) {
if (_followTokenIdByFollowerProfileId[followerProfileId] != 0) {
revert AlreadyFollowing();
}Even if tryMigrate() is called by the protocol team immediately after the V2 upgrade, a malicious user can still call follow() before tryMigrate() by:
- Front-running the migration transaction.
- Holding his profile and follow NFT in different addresses, which causes
tryMigrate()to return here.
As a profile should not be able to follow the same profile twice, tryMigrate() should then revert for old followers who have called follow(). However, this isn't enforced by tryMigrate() as there is no check that _followDataByFollowTokenId[followerProfileId] is 0.
As a result, if tryMigrate() is called after follow(), _followerCount will be incremented twice for a single profile:
unchecked {
++_followerCount;
}
_followTokenIdByFollowerProfileId[followerProfileId] = followTokenId;Additionally, even though _followTokenIdByFollowerProfileId points to a new followTokenId, _followDataByFollowTokenId will not be cleared for the previous follow token ID.
As the state of the FollowNFT contract is now corrupt, followers can perform functions that they normally should not be able to, such as unfollowing when their profile is not a follower (isFollowing() returns false).
Users who are followers before the V2 upgrade will be able to follow with a single profile twice, causing followerCount to be higher than the actual number of profiles following.
Addtionally, as _followDataByFollowTokenId is corrupted, followers might be able to call functions when they should not be allowed to, potentially leading to more severe impacts.
The Foundry test below demonstrates that tryMigrate() can be called although the user is already following, and how followerCount and _followDataByFollowTokenId will be corrupted as a result. It can be run with the following command:
forge test --match-test testCanMigrateWhileFollowing -vvv
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;
import 'test/base/BaseTest.t.sol';
contract FollowMigration_POC is BaseTest {
address target = address(0x1337);
address ALICE;
uint256 targetProfileId;
uint256 aliceProfileId;
uint256 oldFollowTokenId;
function setUp() public override {
super.setUp();
// Setup addresses for Alice
ALICE = makeAddr("Alice");
// Create profile for target and Alice
targetProfileId = _createProfile(target);
aliceProfileId = _createProfile(ALICE);
// Add simulateV1Follow() helper function to FollowNFT implementation
FollowNFTHelper implementation = new FollowNFTHelper(address(hub));
vm.etch(hub.getFollowNFTImpl(), address(implementation).code);
// Follow and unfollow to deploy target's FollowNFT contract
vm.startPrank(defaultAccount.owner);
hub.follow(
defaultAccount.profileId,
_toUint256Array(targetProfileId),
_toUint256Array(0),
_toBytesArray('')
);
hub.unfollow(defaultAccount.profileId, _toUint256Array(targetProfileId));
vm.stopPrank();
// Get FollowNFT contract
address followNFTAddress = hub.getProfile(targetProfileId).followNFT;
followNFT = FollowNFT(followNFTAddress);
// Alice follows target before the V2 upgrade
oldFollowTokenId = FollowNFTHelper(followNFTAddress).simulateV1Follow(ALICE);
}
function testCanMigrateWhileFollowing() public {
// After the V2 upgrade, Alice calls follow() instead of migrating her follow
vm.startPrank(ALICE);
uint256 followTokenId = hub.follow(
aliceProfileId,
_toUint256Array(targetProfileId),
_toUint256Array(0),
_toBytesArray('')
)[0];
// Alice migrates her V1 follow even though her profile is already following
hub.batchMigrateFollows(
_toUint256Array(aliceProfileId),
_toUint256Array(targetProfileId),
_toUint256Array(oldFollowTokenId)
);
// followTokenId's followerProfileId points to Alice's profile
assertEq(followNFT.getFollowerProfileId(followTokenId), aliceProfileId);
// However, Alice's _followTokenIdByFollowerProfileId points to oldFollowTokenId
assertEq(followNFT.getFollowTokenId(aliceProfileId), oldFollowTokenId);
// Follower count is 2 although Alice is the only follower
assertEq(followNFT.getFollowerCount(), 2);
// Wrap both follow tokens
vm.startPrank(ALICE);
followNFT.wrap(followTokenId);
followNFT.wrap(oldFollowTokenId);
// Alice unfollows using removeFollower()
vm.expectEmit();
emit Events.Unfollowed(aliceProfileId, targetProfileId, 1);
followNFT.removeFollower(followTokenId);
// Alice is no longer following
assertFalse(followNFT.isFollowing(aliceProfileId));
// However, she is still able to unfollow for a second time
vm.expectEmit();
emit Events.Unfollowed(aliceProfileId, targetProfileId, 1);
followNFT.removeFollower(oldFollowTokenId);
}
}
contract FollowNFTHelper is FollowNFT {
constructor(address hub) FollowNFT(hub) {}
/*
Helper function to mimic a V1 follow, which does the following:
- Increment _tokenIdCounter
- Mint a followNFT
*/
function simulateV1Follow(address follower) external returns (uint256 followTokenId) {
followTokenId = ++_lastFollowTokenId;
_mint(follower, followTokenId);
}
} function tryMigrate(
uint256 followerProfileId,
address followerProfileOwner,
uint256 idOfProfileFollowed,
uint256 followTokenId
) external onlyHub returns (uint48) {
+ if (_followTokenIdByFollowerProfileId[followerProfileId] != 0) {
+ return 0;
+ }
+
// Migrated FollowNFTs should have `originalFollowTimestamp` set
if (_followDataByFollowTokenId[followTokenId].originalFollowTimestamp != 0) {
return 0; // Already migrated
}In the protocol, publications are uniquely identified through the publisher's profile ID and the publication's ID. For example, when a user calls act(), the publication being acted on is determined by publicationActedProfileId and publicationActedId:
Types.Publication storage _actedOnPublication = StorageLib.getPublication(
publicationActionParams.publicationActedProfileId,
publicationActionParams.publicationActedId
);However, as publication IDs are not based on the publication's data, this could cause users to act on the wrong publication in the event a blockchain re-org occurs.
For example:
- Assume the following transactions occur in separate blocks:
- Block 1: Alice calls
post()to create a post; its publication ID is 20. - Block 2: Bob is interested in the post, he calls
act()withpublicationActedId = 20to act on the post. - Block 3: Alice calls
comment()separately, which creates another publication; its publication ID is 21.
- Block 1: Alice calls
- A blockchain re-org occurs; block 1 is dropped in place of block 3:
- Alice's comment now has the publication ID 20 instead of 21.
- Bob's call to
act()in block 2 is applied on top of the re-orged blockchain:- This causes him to act on the comment instead of the post he intended to, as it now has the publication ID 20.
In this scenario, due to the blockchain re-org, Bob calls act() on a different publication than the one he wanted. This could have severe impacts depending on the action module being called; if the action module is used to collect and pay fees to the publisher and referrals (eg. MultirecipientFeeCollectModule.sol), Bob could have lost funds.
Note that this also applies to comment(), mirror and quote(), as they can be called with reference modules with sensitive logic as well.
If a blockchain re-org occurs, users could potentially act/comment/mirror/quote on the wrong publication, which has varying impacts depending on the action or reference module being used, such as a loss of funds due to paying fees.
Given that Lens Protocol is deployed on Poylgon, which has experienced large re-orgs in the past, the likelihood of the scenario described above occuring due to a blockchain re-org is not low.
Consider identifying publications with a method that is dependent on its contents. For example, users could be expected to provide the keccak256 hash of a publication's contents alongside its publication ID.
This would prevent users from acting on the wrong publication should a publication's contents change despite having the same ID.
In FollowNFT.sol, whenever a follower is blocked by a profile and his followTokenId is unwrapped, processBlock() will mint the follow NFT to the follower's address:
uint256 followTokenId = _followTokenIdByFollowerProfileId[followerProfileId];
if (followTokenId != 0) {
if (!_isFollowTokenWrapped(followTokenId)) {
// Wrap it first, so the user stops following but does not lose the token when being blocked.
_mint(IERC721(HUB).ownerOf(followerProfileId), followTokenId);
}However, if the profile owner's address isn't able to follow NFTs, such as profiles held in a secure wallet (e.g. hardware wallet or multisig), the minted follow NFT would become permanently stuck.
Consider allowing the follower to recover the NFT by himself by assigning profileIdAllowedToRecover to followerProfileId:
uint256 followTokenId = _followTokenIdByFollowerProfileId[followerProfileId];
if (followTokenId != 0) {
if (!_isFollowTokenWrapped(followTokenId)) {
// Wrap it first, so the user stops following but does not lose the token when being blocked.
- _mint(IERC721(HUB).ownerOf(followerProfileId), followTokenId);
+ _followDataByFollowTokenId[followTokenId].profileIdAllowedToRecover = followerProfileId;
}Throughout FollowNFT.sol, regular ERC-721 approvals are not used for access controls. For example, these are the access control checks when follow() is called with a wrapped follow token:
bool isFollowApproved = _followApprovalByFollowTokenId[followTokenId] == followerProfileId;
address followerProfileOwner = IERC721(HUB).ownerOf(followerProfileId);
if (
!isFollowApproved &&
followTokenOwner != followerProfileOwner &&
followTokenOwner != transactionExecutor &&
!isApprovedForAll(followTokenOwner, transactionExecutor) &&
!isApprovedForAll(followTokenOwner, followerProfileOwner)
) {
revert DoesNotHavePermissions();
}Apart from follow approvals, the function only allows the token owner or approved operators (address approved using setApprovalForAll()). If a user is approved by the token owner using approve() he will be unable to call follow() despite having control over the token.
This isn't a major issue as the approved address can sidestep this by doing the following:
- Transfer the follow NFT to himself.
- Call
follow(). - Transfer the follow NFT back to the original address.
However, this could potentially be extremely inconvenient for the approved address.
Consider allowing addresses approved using approve() to call the following functions as well:
follow()unfollow()removeFollower()approveFollow()
In the LensHub contract, users can provide signatures to allow relayers to call functions on their behalf in meta-transactions, such as followWithSig().
However, there is currently no way for the user to revoke their signatures. This could become a problem is the user needs to revoke their signatures (eg. action or reference module turns malicious, user doesn't want to use the module anymore).
In the Lenshub contract, implement a way for users to revoke their signatures. One way of achieving this is to add a function that increments the caller's nonce:
function incrementNonce() external {
StorageLib.nonces()[msg.sender]++;
}In LensBaseERC721.sol, the ERC721Enumerable extension is no longer supported. This can be seen in its supportsInterface() function, which no longer checks for type(IERC721Enumerable).interfaceId:
function supportsInterface(bytes4 interfaceId) public view virtual override(ERC165, IERC165) returns (bool) {
return
interfaceId == type(IERC721).interfaceId ||
interfaceId == type(IERC721Timestamped).interfaceId ||
interfaceId == type(IERC721Burnable).interfaceId ||
interfaceId == type(IERC721MetaTx).interfaceId ||
interfaceId == type(IERC721Metadata).interfaceId ||
super.supportsInterface(interfaceId);
}As LensBaseERC721 is inherited by LensProfiles, profiles will no longer support the ERC721Enumerable extension. This might break the functionality of other protocols that rely on ERC721Enumerable's functions.
Consider documenting/announcing that the V2 upgrade will remove ERC721Enumerable functionality from profiles.
When profiles are transferred, its delegated executor config is not cleared. Instead, the function switches its config to a new config:
function _beforeTokenTransfer(
address from,
address to,
uint256 tokenId
) internal override whenNotPaused {
if (from != address(0) && _hasTokenGuardianEnabled(from)) {
// Cannot transfer profile if the guardian is enabled, except at minting time.
revert Errors.GuardianEnabled();
}
// Switches to new fresh delegated executors configuration (except on minting, as it already has a fresh setup).
if (from != address(0)) {
ProfileLib.switchToNewFreshDelegatedExecutorsConfig(tokenId);
}This could potentially be dangerous as users are able to switch back to previous configs using changeDelegatedExecutorsConfig(). If a previous owner had added himself to previous configs, switching back to a previous config might potentially give the previous owner the ability to steal the profile, or execute malicious functions as a delegated executor.
Consider warning users about the danger of switching to previous configs in the documentation.
According to the Referral System Rules, mirrors cannot be a target publication. However, the act() function does not ensure that the target publication is not a mirror.
This is currently unexploitable as mirrors cannot be initialized with action modules, therefore the _isActionEnabled check will always revert when called with a mirror. However, this could potentially become exploitable if an attacker finds a way to corrupt the enabledActionModulesBitmap of a mirror publication.
Consider validating that the target publication is not a mirror in act().
In GovernanceLib.sol, the setState() function is used by both the emergency admin and governance to change the state of the contract:
function setState(Types.ProtocolState newState) external {
// NOTE: This does not follow the CEI-pattern, but there is no interaction and this allows to abstract `_setState` logic.
Types.ProtocolState prevState = _setState(newState);
// If the sender is the emergency admin, prevent them from reducing restrictions.
if (msg.sender == StorageLib.getEmergencyAdmin()) {
if (newState <= prevState) {
revert Errors.EmergencyAdminCanOnlyPauseFurther();
}
} else if (msg.sender != StorageLib.getGovernance()) {
revert Errors.NotGovernanceOrEmergencyAdmin();
}As seen from above, the emergency admin can only pause further, whereas governance can set any state.
However, if emergency admin and governance happens to be the same address, msg.sender == StorageLib.getEmergencyAdmin() will always be true, which only allows the system to be paused further. This could become a problem if Lens Protocol decides to set both to the same address.
Consider making the following change to the if-statement:
function setState(Types.ProtocolState newState) external {
// NOTE: This does not follow the CEI-pattern, but there is no interaction and this allows to abstract `_setState` logic.
Types.ProtocolState prevState = _setState(newState);
// If the sender is the emergency admin, prevent them from reducing restrictions.
- if (msg.sender == StorageLib.getEmergencyAdmin()) {
+ if (msg.sender != StorageLib.getGovernance() && msg.sender == StorageLib.getEmergencyAdmin()) {
- if (newState <= prevState) {
- revert Errors.EmergencyAdminCanOnlyPauseFurther();
- }
- } else if (msg.sender != StorageLib.getGovernance()) {
- revert Errors.NotGovernanceOrEmergencyAdmin();
- }When an action module is whitelisted, incrementMaxActionModuleIdUsed() is called, which increments _maxActionModuleIdUsed and checks that it does not exceed MAX_ACTION_MODULE_ID_SUPPORTED:
function incrementMaxActionModuleIdUsed() internal returns (uint256) {
uint256 incrementedId;
assembly {
incrementedId := add(sload(MAX_ACTION_MODULE_ID_USED_SLOT), 1)
sstore(MAX_ACTION_MODULE_ID_USED_SLOT, incrementedId)
}
if (incrementedId > MAX_ACTION_MODULE_ID_SUPPORTED) {
revert Errors.MaxActionModuleIdReached();
}
return incrementedId;
}MAX_ACTION_MODULE_ID_SUPPORTED is currently declared as 255:
uint256 constant MAX_ACTION_MODULE_ID_SUPPORTED = 255;This becomes an issue as _maxActionModuleIdUsed does not changed when action modules are un-whitelisted, which means that action module IDs cannot be reused:
// The action module with the given address was already whitelisted before, it has an ID already assigned.
StorageLib.actionModuleWhitelistData()[actionModule].isWhitelisted = whitelist;
id = actionModuleWhitelistData.id;This means that only 255 action modules can ever be whitelisted, and governance will not be able to whitelist action modules forever after this limit is exceeded.
When createProfile() is called, the profile's follow module is initialized with msg.sender as its transactionExecutor:
followModuleReturnData = _initFollowModule({
profileId: profileId,
transactionExecutor: msg.sender,
followModule: createProfileParams.followModule,
followModuleInitData: createProfileParams.followModuleInitData
});This could be extremely limiting if the follow module uses transactionExecutor during its initialization.
For example, consider a follow module that needs to be initialized with a certain amount of tokens:
- If these tokens are transferred from
transactionExecutor, the profile creator will bear the cost of the initialization. - If the profile's owner is meant to provide funds, he will have to transfer the tokens to the profile creator, which creates risk as he has to trust that the profile creator won't rug.
Consider setting transactionExecutor to createProfileParams.to instead, which is the owner of the newly created profile:
followModuleReturnData = _initFollowModule({
profileId: profileId,
- transactionExecutor: msg.sender,
+ transactionExecutor: createProfileParams.to,
followModule: createProfileParams.followModule,
followModuleInitData: createProfileParams.followModuleInitData
});In LensHandles.sol, the _validateLocalNameMigration() function is used to validate migrated handles:
bytes1 firstByte = localNameAsBytes[0];
if (firstByte == '-' || firstByte == '_') {
revert HandlesErrors.HandleFirstCharInvalid();
}
uint256 i;
while (i < localNameLength) {
if (!_isAlphaNumeric(localNameAsBytes[i]) && localNameAsBytes[i] != '-' && localNameAsBytes[i] != '_') {
revert HandlesErrors.HandleContainsInvalidCharacters();
}
unchecked {
++i;
}
}As seen from above, handles can only be migrated if:
- Its first byte is alphanumeric.
- Only contains alphanumeric characters, '-' or "_".
Additionally, the _migrateProfile() function in MigrationLib.sol removes the last 5 bytes of each handle:
assembly {
let handle_length := mload(handle)
mstore(handle, sub(handle_length, DOT_LENS_SUFFIX_LENGTH)) // Cut 5 chars (.lens) from the end
}This means that any V1 handle that does not contan ".lens" and is shorter than 5 bytes cannot be migrated.
However, the rules mentioned above are not strictly enforced in Lens Protocol V1:
function _validateHandle(string calldata handle) private pure {
bytes memory byteHandle = bytes(handle);
if (byteHandle.length == 0 || byteHandle.length > Constants.MAX_HANDLE_LENGTH)
revert Errors.HandleLengthInvalid();
uint256 byteHandleLength = byteHandle.length;
for (uint256 i = 0; i < byteHandleLength; ) {
if (
(byteHandle[i] < '0' ||
byteHandle[i] > 'z' ||
(byteHandle[i] > '9' && byteHandle[i] < 'a')) &&
byteHandle[i] != '.' &&
byteHandle[i] != '-' &&
byteHandle[i] != '_'
) revert Errors.HandleContainsInvalidCharacters();
unchecked {
++i;
}
}
}As seen from above, V1 handles can also contain ".", and its first byte is not only restricted to alphanumeric characters. Additionally, handles do not have to end with the ".lens" suffix. This means that it might be possible to create a handle that cannot be migrated after the V2 upgrade.
Ensure all profile handles obey the following rules before the V2 upgrade:
- First byte is alphanumeric.
- Only contains alphanumeric characters, '-' or "_".
- Ends with the ".lens" suffix.
When a handle is minted, its tokenId is computed as the keccak256 hash of the handle name:
function getTokenId(string memory localName) public pure returns (uint256) {
return uint256(keccak256(bytes(localName)));
}However, _mintHandle() does not ensure that the tokenId minted is not 0:
function _mintHandle(address to, string calldata localName) internal returns (uint256) {
uint256 tokenId = getTokenId(localName);
_mint(to, tokenId);
_localNames[tokenId] = localName;
emit HandlesEvents.HandleMinted(localName, NAMESPACE, tokenId, to, block.timestamp);
return tokenId;
}This makes it theoretically possible to mint a handle with a tokenId of 0, which is problematic as tokenId == 0 is treated as a default value in the protocol. For example, getDefaultHandle() in TokenHandleRegistry returns profile ID as 0 when the handle tokenId is 0:
TokenHandleRegistry.sol#L100-L102
if (resolvedTokenId == 0 || !ILensHub(LENS_HUB).exists(resolvedTokenId)) {
return 0;
}In _mintHandle(), consider checking that tokenId is non-zero to protect against the extremely unlikely event where a localName results in tokenId = 0:
function _mintHandle(address to, string calldata localName) internal returns (uint256) {
uint256 tokenId = getTokenId(localName);
+ if (tokenId == 0) revert InvalidHandle();
_mint(to, tokenId);
_localNames[tokenId] = localName;
emit HandlesEvents.HandleMinted(localName, NAMESPACE, tokenId, to, block.timestamp);
return tokenId;
}The LensHub contract supports the relaying of calls for several functions using a supplied signature. For example, users can provide a signature alongside the usual parameters in setProfileMetadataURIWithSig() for a relayer to call the function on his behalf:
function setProfileMetadataURIWithSig(
uint256 profileId,
string calldata metadataURI,
Types.EIP712Signature calldata signature
) external override whenNotPaused onlyProfileOwnerOrDelegatedExecutor(signature.signer, profileId) {However, the parameters above do not include a gas parameter, which means the relayer can specify any gas amount.
If the provided gas amount is insufficient, the entire transaction will revert. However, if the function exhibits different behaviour depending on the supplied gas (which might occur in modules), a relayer could potentially influence the outcome of the call by manipulating the gas amount.
For all functions used for meta-transactions, consider adding a gas parameter that allows users to specify the gas amount the function should be called with. This gas parameter should be included in the hash digest when verifying the user's signature.
In FollowNFT.sol, approveFollow() only allows profile IDs that exist to be set as followerProfileId:
function approveFollow(uint256 followerProfileId, uint256 followTokenId) external override {
if (!IERC721Timestamped(HUB).exists(followerProfileId)) {
revert Errors.TokenDoesNotExist();
}As seen from above, the function does not allow followerProfileId to be 0. This forces users to set followerProfileId to their own address if they wish to cancel a follow approval, which could pose problems.
Allow approveFollow() to be called withfollowerProfileId = 0 as profiles will never have the ID 0:
function approveFollow(uint256 followerProfileId, uint256 followTokenId) external override {
- if (!IERC721Timestamped(HUB).exists(followerProfileId)) {
+ if (followerProfileId != 0 && !IERC721Timestamped(HUB).exists(followerProfileId)) {
revert Errors.TokenDoesNotExist();
}In LensBaseERC721.sol, the _isApprovedOrOwner() functions contains an if-statement that checks if tokenId exists (_tokenData[tokenId].owner != 0):
function _isApprovedOrOwner(address spender, uint256 tokenId) internal view virtual returns (bool) {
if (!_exists(tokenId)) {
revert Errors.TokenDoesNotExist();
}
address owner = ownerOf(tokenId);
return (spender == owner || getApproved(tokenId) == spender || isApprovedForAll(owner, spender));
}However, this check is redundant as ownerOf also contains the same check:
function ownerOf(uint256 tokenId) public view virtual override returns (address) {
address owner = _tokenData[tokenId].owner;
if (owner == address(0)) {
revert Errors.TokenDoesNotExist();
}
return owner;
}Consider removing the if statement from _isApprovedOrOwner():
function _isApprovedOrOwner(address spender, uint256 tokenId) internal view virtual returns (bool) {
- if (!_exists(tokenId)) {
- revert Errors.TokenDoesNotExist();
- }
address owner = ownerOf(tokenId);
return (spender == owner || getApproved(tokenId) == spender || isApprovedForAll(owner, spender));
}In LensProfiles.sol, the burn() function has the whenNotPaused modifier:
function burn(uint256 tokenId)
public
override(LensBaseERC721, IERC721Burnable)
whenNotPausedHowever, the modifier is redundant as the _beforeTokenTransfer hook also has the whenNotPaused modifier:
function _beforeTokenTransfer(
address from,
address to,
uint256 tokenId
) internal override whenNotPaused {Consider removing the whenNotPaused modifier from burn():
function burn(uint256 tokenId)
public
override(LensBaseERC721, IERC721Burnable)
- whenNotPausedIn the LensHub contract, unfollow() will revert if the profile to unfollow is burnt, due to the following check:
function unfollow(
uint256 unfollowerProfileId,
address transactionExecutor,
uint256[] calldata idsOfProfilesToUnfollow
) external {
uint256 i;
while (i < idsOfProfilesToUnfollow.length) {
uint256 idOfProfileToUnfollow = idsOfProfilesToUnfollow[i];
ValidationLib.validateProfileExists(idOfProfileToUnfollow); // auditor: This checkHowever, this implementation seems incorrect as users should be able to unfollow burnt profiles.
This issue can be sidestepped by calling removeFollower() or burn() in the burnt profile's FollowNFT contract, but could be extremely inconvenient as users will have to first wrap their follow token.
Consider removing the validateProfileExists() check in unfollow():
function unfollow(
uint256 unfollowerProfileId,
address transactionExecutor,
uint256[] calldata idsOfProfilesToUnfollow
) external {
uint256 i;
while (i < idsOfProfilesToUnfollow.length) {
uint256 idOfProfileToUnfollow = idsOfProfilesToUnfollow[i];
- ValidationLib.validateProfileExists(idOfProfileToUnfollow); // auditor: This checkThis is possible as the followNFT addrress of non-existent profiles (profiles that are not minted yet) is 0, which causes the function to revert later on during execution.