SignatureCheckerLib.isValidSignatureNow will always return true if address(0) is an owner meaning malicious users can make arbitrary calls on an account, including in the current set up of the CoinbaseSmartWallet proxy implementation

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/main/src/SmartWallet/CoinbaseSmartWallet.sol#L104-L106
https://github.com/code-423n4/2024-03-coinbase/blob/main/src/SmartWallet/MultiOwnable.sol#L85

Vulnerability details

Impact
There are no checks in MultiOwnable::addOwnerAddress to stop someone adding the zero address as an owner. While typically this would not be a significant issue, in the case of this codebase if address(0) is an owner CoinbaseSmartWallet::_validateSignature will always return true regardless of the signature if SignatureWrapper.ownerIndex is the index of the zero address.

This means that a malicious user would be able to use EntryPoint transactions to cause significant damage to a users account (steal funds, change owners, upgrade the proxy implementation).

This is currently the case in the CoinbaseSmartWallet proxy implementation contract as address(0) is mistakenly added as an owner in an attempt to make the proxy implementation inoperable, but instead has the exact same effect. This would escalate the issue severity to critical if it were possible to do something such as upgrade the implementation contract's implementation, putting all CoinbaseSmartWallet instances that use this implementation contract at risk.

Proof of Concept
The following shows the control flow outlining how any instance where address(0) is a viable owner will result in the signature being validated:

Recommended Mitigation
Given the severity of the damage should the zero address be made an owner, and that the likelihood of the issue occurring is scaled up by the number of users using a CoinbaseSmartWallet, the project should implement sufficient address(0) checks when MultiOwnable::addOwnerAddress to protect their users from a potentially costly error.

On top of this the CoinbaseSmartWallet constructor should not pass address(0) as an owner when calling _initializeOwners and should instead use another address such as "0x000000000000000000000000000000000000dEaD" top stop users being able to make arbitrary calls via the proxy implementation contract.

Assessed type

Other

[raymondfam]

See #90.

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #90

[3docSec]

Interesting finding. If we exclude user errors, it is safe to assume that wallets created by the factory are initialized with non-zero addresses because the factory creates and initializes wallets in the same call.

The only exception to the above is the implementation contract, that has its storage initialized in the constructor. For this one, the only consequence I can imagine is that the implementation wallet can accept UserOperations from anybody. This does not seem to be a big deal, apart from self-calls to upgradeToAndCall which are however secured by the onlyProxy modifier.

The similar finding (this and this) pointed by #90 as having previously been judged valid are much more impactful because they open a viable path for destructing the implementation - this is not true for this codebase.

Leaving this as QA because having address(1) as the implementation owner is a sound recommendation that would prevent the implementation contract from accepting UserOperations with malformed signatures.

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-a

[wilsoncusack]

This is an interesting one, thanks for bring it up. This would have been very concerning if there were a way for the implementation to delegatecall, and an attacker could selfdestructed the contract. I tried poc here, trying to call upgradeToAndCall and you get the error UnauthorizedCallContext from Solady UUPS.

[wilsoncusack]

Looking further, I do not think this finding is correct re Solady behavior https://github.com/Vectorized/solady/blob/main/test/SignatureCheckerLib.t.sol#L68-L70