Token guardian protection doesn't account for approved operators in `approve()`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/namespaces/LensHandles.sol#L139-L145

Vulnerability details

Bug Description

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:

LensHandles.sol#L139-L145

    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:

ERC721.sol#L116-L119

        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.

Note that the approve() function in LensProfiles.sol also has the same vulnerability.

Impact

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.

Proof of Concept

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

Recommended Mitigation

Consider checking if the token's owner has token guardian enabled as well:

LensHandles.sol#L139-L145

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

Assessed type

Access Control

[c4-pre-sort]

141345 marked the issue as duplicate of #90

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report