AddOwner functions lack validation, risking invalid owner data and transaction failures.

[c4-bot-1]

Lines of code

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

Vulnerability details

Overview

MultiOwnable contract allows adding owners by providing an Ethereum address or a WebAuthn public key. However, there is a lack of proper input validation when adding owners, which could potentially lead to the addition of invalid or malformed owner data. This may result in signature validation failures for the affected owners, effectively bricking their ability to execute transactions.

Description

addOwnerAddress and addOwnerPublicKey, which are responsible for adding new owners to the SmartWallet contract.

addOwnerAddress function:

function addOwnerAddress(address owner) public virtual onlyOwner {
    _addOwner(abi.encode(owner));
}

addOwnerPublicKey function:

function addOwnerPublicKey(bytes32 x, bytes32 y) public virtual onlyOwner {
    _addOwner(abi.encode(x, y));
}

Both functions take the owner data (Ethereum address or WebAuthn public key) as input, encode it using abi.encode, and pass it to the internal _addOwner function.

The issue lies in the lack of proper input validation when adding owners using the addOwnerAddress and addOwnerPublicKey functions.

In the addOwnerAddress function, there is no explicit validation to ensure that the provided owner parameter is a valid Ethereum address. Similarly, in the addOwnerPublicKey function, there is no validation to check if the provided x and y parameters form a valid WebAuthn public key.

#L86

_addOwner(abi.encode(owner));

and #L94

_addOwner(abi.encode(x, y));

These lines encode the owner data without any prior validation and pass it to the _addOwner function, which adds the owner to the contract's storage.

The expected behavior is that only valid Ethereum addresses and WebAuthn public keys should be accepted as owners when calling the addOwnerAddress and addOwnerPublicKey functions, respectively. The functions should perform proper input validation to ensure the integrity and validity of the owner data before adding it to the contract.

Due to the lack of validation, the addOwnerAddress and addOwnerPublicKey functions allow the addition of invalid or malformed owner data. For example, an invalid Ethereum address or a malformed WebAuthn public key can be added as an owner without any checks or restrictions.

When attempting to execute transactions using the invalid owner data, the signature validation process in the _validateSignature function will fail. This is because the _validateSignature function expects the owner data to be in a specific format (32 bytes for Ethereum address or 64 bytes for WebAuthn public key) and performs certain checks on the data.

If the owner data is invalid or malformed, the signature validation will always fail for that specific owner, effectively bricking their ability to execute transactions.

Impact

Owners with invalid Ethereum addresses or malformed WebAuthn public keys will be unable to execute transactions, as the signature validation will always fail for them.

Recommended Mitigation Steps

For addOwnerAddress function:

function addOwnerAddress(address owner) public virtual onlyOwner {
+      require(owner != address(0), "Invalid Ethereum address");
    _addOwner(abi.encode(owner));
}

For addOwnerPublicKey function:

function addOwnerPublicKey(bytes32 x, bytes32 y) public virtual onlyOwner {
+      require(isValidWebAuthnPublicKey(x, y), "Invalid WebAuthn public key");
    _addOwner(abi.encode(x, y));
}

function isValidWebAuthnPublicKey(bytes32 x, bytes32 y) internal pure returns (bool) {
    // Implement WebAuthn public key validation logic
    // Example: Check if the point (x, y) is on the secp256r1 curve
    // ...
}

These measures ensure that only valid Ethereum addresses and WebAuthn public keys are accepted as owners. The addOwnerAddress function checks if the provided address is not the zero address, while the addOwnerPublicKey function includes a separate validation function (isValidWebAuthnPublicKey) to check the validity of the WebAuthn public key.

Assessed type

Invalid Validation

[raymondfam]

removeOwnerAtIndex can always be used to counter mistakes done through addOwnerAddress and addOwnerPublicKey.

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[3docSec]

QA is more appropriate for user mistakes.

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-a