QA-01 MitigationConfirmed

[c4-bot-2]

Lines of code

Vulnerability details

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/MultiOwnable.sol#L102

Issue Report

QA-01: All Smart Wallet funds will be lost if users remove all owners

Details

Issue#181

An oversight was found in removeOwnerAtIndex where all owners could be removed including the last owner. This presents a significant risk that can potentially lead to loss of funds if all owners loose access.

Mitigation

PR#43

• The modified version of removeOwnerAtIndex now includes a check to ensure that the operation does not proceed if attempting to remove the last owner.

Loc:

function removeOwnerAtIndex(uint256 index, bytes calldata owner) external virtual onlyOwner {
        MultiOwnableStorage storage $ = _getMultiOwnableStorage();
        if ($.nextOwnerIndex - $.removedOwnersCount == 1) {
            revert LastOwner();
        }

        _removeOwnerAtIndex(index, owner);
    }

• A function was added to be able to remove last owner to allow for revocation of last owner if need be.

Loc:

function removeLastOwner(uint256 index, bytes calldata owner) external virtual onlyOwner {
        MultiOwnableStorage storage $ = _getMultiOwnableStorage();
        uint256 ownersRemaining = $.nextOwnerIndex - $.removedOwnersCount;
        if (ownersRemaining > 1) {
            revert NotLastOwner(ownersRemaining);
        }

        _removeOwnerAtIndex(index, owner);
    }

Suggestion

The number 1 is used to check if there's only one owner left. While understandable, using magic numbers directly in code can be considered poor practice. Define a constant at the beginning of your contract to give context to this value.

uint256 private constant MIN_OWNERS = 1;

Conclusion

This fix succesfully mitigates the issue#181

[c4-judge]

3docSec marked the issue as satisfactory

[c4-judge]

3docSec marked the issue as confirmed for report

[c4-judge]

3docSec marked the issue as not confirmed for report