Audit of Self EU ID cards Circuits, OFAC Checks, and Smart Contracts

Description.

The sparse Merkle tree data structure that is used to perform all the OFAC checks support a non-inclusion proof that can be used to prove that a key is not present in the tree. The way a non-inclusion proof is constructed is by providing the opening of a leaf node, which shares the same bit prefix as the key being proven not to be present. In other words, the opened leaf is the “closest” leaf to the key opened. The verifier checks that the provided leaf is included in the tree by checking that the path and the siblings hashes reconstruct correctly the root hash of the tree. Then, the verifier checks that the provided opening leaf is not equal to the key present in the tree.

The verifier circuit first computes the binary representation of the key to be searched in the following way:

// Convert the full key to bits and take only nLength bits
component num2Bits = Num2Bits(254);  // Using 254 bits for poseidon output
num2Bits.in <== virtualKey;

Unfortunately, the Num2Bits(254) component presents an aliasing issue. Looking at the code of Num2Bits in circomlib, the circuit witnesses the binary representation in the out array, and then checks that

template Num2Bits(n) {
    signal input in;
    signal output out[n];
    var lc1=0;

    var e2=1;
    for (var i = 0; i<n; i++) {
        out[i] <-- (in >> i) & 1;
        out[i] * (out[i] -1 ) === 0;
        lc1 += out[i] * e2;
        e2 = e2+e2;
    }

    lc1 === in;
}

This computation is done modulo the field size, which is smaller than $2^{254}$. Therefore, for approximately a quarter of the possible keys, an attacker can witness a different binary representation of the key. More concretely, if the original key is $k$ and the field prime is $p$, if $k<2^{254}-p$ the attacker can witness the binary representation of $k+p$, which will be accepted by the circuit. However, this new representation will not be equal to the original key’s representation, and the new path that will be checked by the circuit will be different. This means that an attacker can craft a fake non-inclusion proof for a key which is present in the tree, by just opening the leaf corresponding to the position of $k+p$ instead of $k$.

Impact.

As a result of this aliasing issue, an attacker can craft a fake non-inclusion proof for a key that is present in the tree. Since every OFAC check currently implemented relies on a non-inclusion proof, this means that the attacker can bypass any OFAC check. Notice that this can be exploited only for keys that are smaller than $2^{254}-p$, which occurs approximately for one quarter of the keys.

Recommendation.

We recommend replacing the Num2Bits component with Num2Bits_strict, which does not have the aliasing issue. We also recommend replacing the later Num2Bits component with Num2Bits_strict in the same file

signal path_in_bits_reversed[nLength] <== Num2Bits(nLength)(smallKey);

or, alternatively, adding an assertion to check that nLength is less or equal than 253, to avoid aliasing issues in case of changes to the nLength parameter in the future.

Client response.

The client has acknowledged the issue and addressed it in commit 99e8eec.

Description.

The configId embedded in the userContextData acts as a policy selector: it points to a specific, owner-registered VerificationConfigV2 struct that encodes which checks (OFAC, forbidden‐countries, age gates, etc.) should be applied during a user’s proof verification. When a proof is submitted, the hub decodes the first 32 bytes as configId, looks up the corresponding config and then enforces those rules in its customVerify logic. In this way, contract owners define the allowed verification behaviors off-chain and users choose which policy to invoke on-chain simply by supplying its configId.

function _executeVerificationFlow(
        SelfStructs.HubInputHeader memory header,
        bytes memory proofData,
        bytes calldata userContextData
    ) internal returns (bytes memory output, uint256 destChainId, bytes memory userDataToPass) {
        bytes32 configId;
        uint256 userIdentifier;
        bytes calldata remainingData;
        {
            uint256 _destChainId;
            (configId, _destChainId, userIdentifier, remainingData) = _decodeUserContextData(userContextData);
            destChainId = _destChainId;
        }

        {
            bytes memory config = _getVerificationConfigById(configId);
            ...

To manage these configurations, in contracts/contracts/IdentityVerificationHubImplV2.sol there is a function setVerificationConfigV2 that defines a verification configuration policy set by the owner. There is also a verificationConfigV2Exists that helps checking if the configuration exists. However there are two issues with the current design. First, in the current logic users can choose the configID they want to, which is potentially different (and weaker) to the one intended for the context. Second, the verificationConfigV2Exists is not used inside the _getVerificationConfigById function. Here, if a malicious user or relayer passes an arbitrary ID it defaults to a struct with all values set to 0 (false) if configID is not defined in the VerificationConfigs map:

function _getVerificationConfigById(bytes32 configId) internal view returns (bytes memory config) {
        IdentityVerificationHubV2Storage storage $v2 = _getIdentityVerificationHubV2Storage();
        SelfStructs.VerificationConfigV2 memory verificationConfig = $v2._v2VerificationConfigs[configId];
        config = GenericFormatter.formatV2Config(verificationConfig);
    }

We have tested this on minimalistic smart contract that implements this function locally and queried it for instance on the non existing ID:

configID = "0x1111111111111111111111111111111111111111111111111111111111111111" 

and obtained:

{
    "0": "tuple(bool,uint256,bool,uint256[4],bool[3]): false,0,false,0,0,0,0,false,false,false"
}

which is the configuration for:

olderThanEnabled: false
olderThan: 0
forbiddenCountriesEnabled: false
forbiddenCountriesListPacked: [0,0,0,0]
ofacEnabled: [false,false,false]

Moreover the existing helper verificationConfigV2Exists is vulnerable to an attack where the malicious user precomputes the hash of the default “all zeroes” configuration, since this will be the output of the mapping for any ID:

    function verificationConfigV2Exists(bytes32 configId) external view virtual onlyProxy returns (bool exists) {
        SelfStructs.VerificationConfigV2 memory config = getVerificationConfigV2(configId);
        return generateConfigId(config) == configId;
    }

In sum, there is an underlying design issue allowing malicious users or relayers to choose a trivial verification configuration. Although the contract provides verificationConfigV2Exists(configId), this helper is incorrectly implemented and it is never invoked, so _getVerificationConfigById blindly returns defaults (“all zeroes”).

Impact.

Given configID comes from userContextData, this vulnerability can be used to completely avoid verification checks based on the “all false” verification configuration. An attacker can thus bypass OFAC screening, country‐ban checks, and age gates simply by choosing an arbitrary configId, because all of the ofacEnabled, forbiddenCountriesEnabled, and olderThanEnabled flags will be false. For instance:

function verifyPassport(
        SelfStructs.VerificationConfigV2 memory verificationConfig,
        SelfStructs.PassportOutput memory passportOutput
    ) internal pure returns (SelfStructs.GenericDiscloseOutputV2 memory) {
        if (
            verificationConfig.ofacEnabled[0] || verificationConfig.ofacEnabled[1] || verificationConfig.ofacEnabled[2]
        ) {
            if (
                !CircuitAttributeHandlerV2.compareOfac(
                    AttestationId.E_PASSPORT,
                    passportOutput.revealedDataPacked,
                    verificationConfig.ofacEnabled[0],
                    verificationConfig.ofacEnabled[1],
                    verificationConfig.ofacEnabled[2]
                )
            ) {
                revert InvalidOfacCheck();
            }
        }
        ....

Recommendation. It is recommended to review the design of the custom configuration to avoid users to choose arbitrary and trivial values for configID. If this is something that is context dependent, it should be possible for the dapp to independently enforce a given configID, without relying on the ID declared by the user, which could be maliciously chosen. It is recommended also to fix and use the existing verificationConfigV2Exists helper to gate lookups and revert early. It is important that the fix keeps track on whether configID has really been set by the owner to verify its existence (for instance by means of an auxiliary mapping on IDs).

Client response. Client has acknowledged the issue, and is working on a fix to prevent malicious users or relayers to choose the configID, and to revert on non-existing IDs.

Description.

The BinaryMerkleRoot template is used in multiple places in the circuits to recover the root of a binary Merkle tree. This template is used to verify the Merkle proof of a leaf in the tree, by checking that the provided path and siblings reconstruct the honest root hash of the tree. The template is implemented as follows.

template BinaryMerkleRoot(MAX_DEPTH) {
    signal input leaf, depth, indices[MAX_DEPTH], siblings[MAX_DEPTH];

    signal output out;

    signal nodes[MAX_DEPTH + 1];
    nodes[0] <== leaf;

    signal roots[MAX_DEPTH];
    var root = 0;

    for (var i = 0; i < MAX_DEPTH; i++) {
        var isDepth = IsEqual()([depth, i]);

        roots[i] <== isDepth * nodes[i];

        root += roots[i];

        var c[2][2] = [ [nodes[i], siblings[i]], [siblings[i], nodes[i]] ];
        var childNodes[2] = MultiMux1(2)(c, indices[i]);

        nodes[i + 1] <== Poseidon(2)(childNodes);
    }

    var isDepth = IsEqual()([depth, MAX_DEPTH]);

    out <== root + isDepth * nodes[MAX_DEPTH];
}

Notice that this template does not enforce any boolean constraints on the indices array. The MultiMux1 component used to compute the inputs to the Poseidon, and is implemented as follows.

template MultiMux1(n) {
    signal input c[n][2];  // Constants
    signal input s;   // Selector
    signal output out[n];

    for (var i=0; i<n; i++) {

        out[i] <== (c[i][1] - c[i][0])*s + c[i][0];

    }
}

This template works correctly when the s input is a boolean value, but it does not enforce that the s input is a boolean value.

As a result, an attacker can craft fake Merkle proofs for any leaf value of their choosing by carefully constructing the indices and siblings array such that the input to the Poseidon call matches a real leaf pair. More concretely, an attacker can choose an arbitrary leaf value and does need to solve the following system of equations over the Circom field:

(sibling - leaf) * index + leaf = honest_left_leaf
(leaf - sibling) * index + sibling = honest_right_leaf

where honest_left_leaf and honest_right_leaf are the two nodes directly below the honest root of the tree. This system of equations is trivial to solve, as it is a linear system with two equations and two unknowns (the index and the sibling). The computed Poseidon hash by the template will be equal to the honest root hash of the tree. At this point, they can use the computed index and sibling as a valid length-1 proof for any leaf of their choosing.

Impact.

As a result of this issue, an attacker can craft a fake Merkle proof for any leaf value of their choosing. Since the Merkle tree is used to perform multiple checks throughout the circuits, this means that the attacker can bypass any check that relies on a Merkle proof, for example the inclusion of the CSCA and DSC certificates in the certificates trees.

Recommendation.

We recommend adding boolean constraints to the indices array in the BinaryMerkleRoot template, to ensure that the MultiMux1 component works correctly.

Client response.

The client addressed this issue independently in commit 8801c6c as a result of a coordinated disclosure with Ethereum Foundation PSE team, which is the owner of the library containing the BinaryMerkleRoot template. After independently discovering the issue, we also initiated a coordinated disclosure with the Ethereum Foundation PSE team, which was already aware of the issue and is planning to address it in a future release of the library.

Description.

In multiple places in the registration and disclosure circuits, there is the usage of LessEqThan template, for example in register_id.circom:

signal dsc_pubKey_offset_in_range <== LessEqThan(12)([
    dsc_pubKey_offset + dsc_pubKey_actual_size,
    raw_dsc_actual_length
]); 
dsc_pubKey_offset_in_range === 1;

There are actually two separate issues with this code.

• The computation dsc_pubKey_offset + dsc_pubKey_actual_size could overflow or underflow the field, leading to a wrong check semantics.
• The LessEqThan template checks that the first input is less than or equal to the second input, but it assumes that both of the inputs are between $0$ and $2^{12}-1$. If this is not the case, the circuit will produce some unexpected behaviour, as for example LessEqThan(12) with input values $p-1$ and $0$ will return $1$ instead of $0$. In this context, the intended range of the indices is to be between $0$ and $2^{12}-1$, so this check could pass even if the indices are outside this range, leading to unexpected behaviour in the circuit.

Impact.

An attacker can provide incorrect indices to the circuit, for example negative indices, which pass the range checks. We could not find any concrete exploit, as incorrect indices make the proof fail in subsequent checks. However, they can lead to unexpected behaviour in the circuits, if for example some other circuits rely on the fact that indices are in the correct range.

Recommendation.

We recommend to ensure that every computation done with indices does not overflow or underflow the field, by adding additional range checks. Additionally, we recommend to add range checks to the input signals of the LessEqThan template, to ensure that there is no unexpected behaviour when the inputs are outside the intended range.

Client response.

Client has acknowledged this issue and addressed it on commit 49de549.

Description. The newly introduced circuits for ID cards are almost identical to the ones for passport, changing only the length of the data group and the positions of some attributes within the data group. This logic is specified in:

• circuits/register/register.circom
• circuits/register_id/register_id.circom
• circuits/disclose/vc_and_disclose.circom
• circuits/disclose/vc_and_disclose_id.circom
• circuits/utils/passport/disclose/disclose.circom
• circuits/utils/passport/disclose/disclose_id.circom

This currently does not pose a security threat, but future changes to this circuits need to be strictly checked across document types, and make more likely the introduction of divergent behaviour between them.

For instance the only substantial difference between register_id.circom and register.circom in 185 lines of code are two lines, namely:

    var attestation_id = 1; /// or =2 for IDs

and:

    var DG1_LEN = 93; /// or =95 for IDs

Recommendation. It is recommended to consider a refactor that keeps the common behaviour in one file, with specializations for the length and positions for various document types.

Client response. Client has acknowledged that this can be done for certain currently duplicated files like the registration circuits, but could be more difficult for others. They will explore this issue further and consider a refactor.

Description.

Several small clean-ups across the codebase can shave gas and tighten maintenance:

• The modifier onlyProxy is applied to the internal function getVerificationConfigV2. Given that all external functions which call getVerificationConfigV2 are covered by the onlyProxy modifier, there is no need for internal functions to use the check as well.

• The IdCardAttributeHandler.sol is superseded by CircuitAttributeHandlerV2 and currently not used.

• In CircuitAttributeHandlerV2 the OFAC field positions for passports is defined as (90,92) but for IDs this is (92,92) instead of (92,93). This does not currently affect the logic since OFAC positions are computed with respect to start position but this can be unified for consistency and avoid issues in future changes.
• In IdentityRegistryIdCardImplV1.sol the import import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; is not necessary given that this is handled by upgradeable/ImplRoot.sol Similarly for IdentityRegistryImplV1.sol and UUPSUpgradeable,Ownable2StepUpgradeable.
• Strings is imported but not used in IdentityRegistryImplV1.sol.

Client response. Client has acknowledged these suggestions and is working on fixes.

Description.

The tests in circuits/tests/disclose/vc_and_disclose_id.test.ts exercise the circuits/circuits/disclose/vc_and_disclose_id.circom circuit. However, there are some bugs that make these tests inaccurate:

In line 144 it is necessary to loop until 90 bytes instead of 88, given the format for ID cards.

        for (let i = 0; i < 88; i++) {
          if (selector_dg1[i] == '1') {
            const char = String.fromCharCode(Number(inputs.dg1[i + 5]));
            assert(reveal_unpacked[i] == char, 'Should reveal the right character');
          } else {
            assert(reveal_unpacked[i] == '\x00', 'Should not reveal');
          }
        }

Similarly, in line 190-191 the checks should be on positions 90 and 91, instead of 88 and 89.

    expect(reveal_unpacked[88]).to.equal('\x00');
    expect(reveal_unpacked[89]).to.equal('\x00');

Recommendation.

Currently, the tests pass because bytes 89 and 90 are not supposed to be revealed in the test. We recommend to fix this nevertheless to allow the test to catch future bugs in this region.

Client response.

Client has acknowledged the issue and patched the tests accordingly on commit bf8cf6c.

Description.

Different sparse Merkle trees are used to perform all the checks related to OFAC. The way they are used is to store the hash of banned entities in the leaves. Then, it is possible to provide a non-inclusion proof for a given key, which proves that the data contained in the passport or ID card are not in the tree.

Currently, the leaves of the sparse Merkle tree store a truncated hash of 64 bits, instead of the full hash of 254 bits. This means that, even though very low, the probability of an honest user colliding with a banned entity is not zero, which can lead to false positives in the OFAC checks.

Impact.

If an honest user data hash collides with a banned entity for the first 64 bits, the honest user will be rejected by the OFAC check. This event occurs with probability

where the number of leaves in the tree is the number of banned entities.

Recommendation.

We recommend changing the sparse Merkle tree implementation to store the full 254-bit hash instead of the truncated 64-bit hash. Additionally, due to the way Merkle proofs are constructed, this change would not require any modifications to the existing circuits or proofs. Indeed, the length of the proof is determined by the largest common prefix shared between two keys in the set, and not by the length of the hash stored in the leaves.

Client response.

Client has acknowledged this issue and, since it is not a critical issue, is considering addressing it in the future by storing the full 254-bit hash in the leaves of the sparse Merkle tree.