Description

The removeOwner function facilitates the removal of owners from one's smart account.

function removeOwner(address owner) external override {

    if (!_smartAccountOwners[owner][msg.sender])
        revert NotAnOwner(owner, msg.sender);
    _transferOwnership(msg.sender, owner, address(0)); 
    unchecked {
        --numberOfOwners[msg.sender];
    }
}

It first checks whether the owner to be removed actually belongs to the smartAccountOwners mapping for the given msg.sender, and then it transfers the ownership from owner to address(0).

function _transferOwnership(
    address smartAccount,
    address owner,
    address newOwner
) internal {
    _smartAccountOwners[owner][smartAccount] = false;
    _smartAccountOwners[newOwner][smartAccount] = true;
    emit OwnershipTransferred(smartAccount, owner, newOwner);
}

This is a workaround solution, as, by default, address(0)'s entry within the smartAccountOwners mapping would point to false. The setting of address(0)'s entry to true allows msg.sender to keep calling removeOwner, which will do the same transfer of ownership to address(0) potentially over and over again and would eventually underflow the numberOfOwner mapping, as the operations performed there are under unchecked.

function removeOwner(address owner) external override {

    if (!_smartAccountOwners[owner][msg.sender])
        revert NotAnOwner(owner, msg.sender);
    _transferOwnership(msg.sender, owner, address(0)); 
    unchecked {
        --numberOfOwners[msg.sender];
    }
}

Impact

The numberOfOwners mapping will eventually underflow. Moreover, the _smartAccountOwners[address(0)][msg.sender] will be set to true, which is an override of the intended and expected value of false, as address(0) should never be an owner.

Recommendations

We recommend performing the delete operation when removing the _smartAccountOwners entry. On top of that, we recommend removing the unchecked operation, as the gas savings are limited over the potential downside of underflows, as exemplified in this issue.

function removeOwner(address owner) external override {

    if (!_smartAccountOwners[owner][msg.sender])
        revert NotAnOwner(owner, msg.sender);
-   _transferOwnership(msg.sender, owner, address(0)); 
+   _smartAccountOwners[owner][msg.sender] = false;

    unchecked {
        --numberOfOwners[msg.sender];
    }
}

Remediation

This issue has been acknowledged by Biconomy, and a fix was implemented in commit bb275bf7↗.