c4-bot-9 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/ERC1271.sol#L100-L111 https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/ERC1271.sol#L143

Vulnerability details

Impact

Possible discrepancies in signature validation.

Proof of Concept

In the ERC1271 contract, the domainSeparator() function is responsible for calculating the domain separator used in EIP-712 compliant hashes. This domain separator is crucial for ensuring the integrity of signature validation processes. However, a vulnerability arises due to the reliance of the domainSeparator() function on the name and version returned by an internal function _domainNameAndVersion().

Let's delve into the code snippet to understand the vulnerability more deeply:

function domainSeparator() public view returns (bytes32) {
    (string memory name, string memory version) = _domainNameAndVersion();
    return keccak256(
        abi.encode(
            keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
            keccak256(bytes(name)),
            keccak256(bytes(version)),
            block.chainid,
            address(this)
        )
    );
}

In this function:

The name and version are obtained by calling _domainNameAndVersion().
The domain separator is then calculated using these values, along with the chain ID and contract address. The vulnerability arises from the fact that if the name or version returned by _domainNameAndVersion() changes after deployment, the domain separator will still be calculated using the old values. This inconsistency in the domain separator calculation can lead to discrepancies in signature validation.

The _domainNameAndVersion() function is abstract and must be implemented by inheriting contracts. It typically returns the name and version of the signing domain. If, for instance, the contract owner decides to upgrade the contract and changes the name or version, the domain separator will continue to be calculated using the old values. This can result in signature validation failures or unexpected behavior in contracts relying on the ERC1271 functionality.

Tools Used

Manual

Recommended Mitigation Steps

Ensure that the name and version used for calculating the domain separator are immutable after deployment.

Assessed type

Context

c4-pre-sort commented 8 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 8 months ago

raymondfam marked the issue as primary issue

raymondfam commented 8 months ago

CoinbaseSmartWallet.sol inheriting ERC1271.so has had _domainNameAndVersion() overridden and hardcoded. Additionally, CoinbaseSmartWallet.sol is upgradeable and will have domainSeparator() dynamically updated when need be:

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/CoinbaseSmartWallet.sol#L333-L335

3docSec commented 7 months ago

The claim If, for instance, the contract owner decides to upgrade the contract and changes the name or version, the domain separator will continue to be calculated using the old values is incorrect. Upgrading the contract will cause the new implementation of _domainNameAndVersion() to be called and the new values to be returned.

c4-judge commented 7 months ago

3docSec marked the issue as unsatisfactory: Invalid

code-423n4 / 2024-03-coinbase-findings

Inconsistent Domain Separator Calculation in ERC1271 Contract #7