Audit of Silent Protocol Smart Contracts
July 7th, 2023

Description. Assuming that zkSecurity is looking at the production-ready smart contracts, the following function that was meant for debugging only was still present and publicly callable by anyone:

/**
 * @notice HERE FOR TESTING PURPOSES ONLY. 
 * TODO: Remove this function before production deployment
*/
function mint(address _recipient, uint256 _amount) public {
    _mint(_recipient, _amount);
}

This allowed any Ethereum user to call it and mint any amount of tokens to themselves.

Recommendation. In general, mixing development and production code is not ideal, as one can easily forget to remove test-only code.

Description. Users of Silent Protocol can subscribe to the smart contract in order to reduce their fees. (Specifically, fees taken from the user when withdrawing.) The smart contract function to do that is registerZeroFee:

function registerZeroFee(
    FeeRegistrationInput calldata _input,
    uint256[8] calldata _proof
) external payable

One of the function argument, _input.validUntil, dictates until when (i.e. what block number) the subscription will be active for. This value is then encrypted and checked (via the use of proofs) when the user withdraws.

There are no checks on the discussed user-controlled value in the smart contract logic. This allows users to enter arbitrary values for _input.validUntil, for example, to get a lifetime subscription in exchange for a small one-time fee.

Note that there is a subscriptionDuration value stored in the state of the smart contract, but it's currently unused.

Recommendation. Use subscriptionDuration to ensure that the validUntil value set by the user is valid. In addition, add a negative test to test that a user can't subscribe for too long.

Client Response. Silent Protocol's monorepo did not contain the issue, only the contracts that were shared with zkSecurity did.

Description. Silent Core uses the Baby Jubjub curve for user keypairs, as well as the compliance committee keypairs. The Baby Jubjub curve, defined in EIP 2494, is an elliptic curve that is native to the circuit field of Ethereum's BN254 pairing curve.

Baby Jubjub is a twisted Edwards curve, which means that the equation of the curve is of the form $ax^2 + y^2 = 1 + dx^2y^2$ for some specified $a$ and $d$, and over a specified field $\mathbb{F}_r$.

One particularity of such curves, is that for a fixed $y$-coordinate, one can recover two valid points $(x, y)$ and $(-x, y)$ on the curve.

For this reason, the $y$ coordinate alone is not enough to recover a known point, and the logic often carries additional information (the sign of the $x$ coordinate).

In this finding we explain how this ambiguity can be exploited to brick the AML public key in the Compliance.sol smart contract, and how it can be fixed.

How the Silent Protocol handles Baby Jubjub points. There are different ways that the smart contracts of the Silent Protocol handles these points:

• SMASP.sol stores them using the packed representation, but often handles them with the sign of the $x$ coordinate separately. Specifically, the user always provides the sign of $x$. As far as we've looked, it seems that since everything stored in the smart contract is in packed representation, no issues arise from this ambiguity.
• SilentSanctionsList.sol stores Baby Jubjub points as $y$ coordinates without the sign of $x$. This means that for a given Silent public key $(x, y)$, the related public key $(-x, y)$ will also be sanctioned. Which seems to be fine as the two addresses are related (if you know the private key of one, you can trivially derive the private key of the other).
• Compliance.sol mixes both storage in uncompressed format (i.e. $(x, y)$) as well as in $y$-coordinate.

The latter case is problematic as it leads to ambiguous scenarios. We want to avoid situations where a proof for one key can be reused for another, or a replay for the same key due to using a related key, or a wrong computation is accepted due to using a related key, etc.

One example that seems to be exploitable is in the compliance smart contract. When a committee member joins the committee, they register their public key with the register() function. This function stores the uncompressed public key ($(x, y)$).

Later, the member adds their contribution to the AML public key by calling shareSecret(). At this point, they can call it with the related key ($(-x, y)$) but with a proof that uses the original key ($(x, y)$).

Note that anyone can observe a valid use of shareSecret() by a committee member, and attempt to front-run the transaction using the same proof but with an invalid point.

The previous malicious transaction will successfully execute (as demonstrated by the reproduction steps below), and the wrong elliptic curve point will be added to the AML key. Meaning that the secret share (along with its distributed shares encrypted to other members) will be uncorrelated with the AML public key, thus transitioning the compliance contract into an unrecoverable state if that were to happen.

See the code of shareSecret() commented below:

function shareSecret(
    SecretSharingInput calldata _input,
    uint256[8] memory _proof,
    uint256[2] memory publicKey
) external {
    // TRUNCATED...

    // check if the proof is valid (this can use the original key)
    require(
        Verifier.verifySecretSharing(
            pkIndexes,
            _input,
            _proof,
            verifyingKeys_2
        ),
        "Invalid proof"
    );

    // TRUNCATED...

    if (!isPublicKeyAdded[yPublicKey]) {
        // this will add the related key to the AML public key
        updatePublicKeyJoin(publicKey);
        isPublicKeyAdded[yPublicKey] = true;
    }

Reproduction steps. You can add the following test to test/compliance.test.ts to reproduce the issue:

it("should reject if (-x, y) used instead of (x, y)", async function () {
    const { compliance, crypto } = await loadFixture(setup);

    // register a user
    let pks = [];
    const user = await crypto.genKeyPair();
    pks.push({
        xPublicKey: user.publicKey[0],
        yPublicKey: user.publicKey[1],
        index: bigint2array(genRandomScalar()),
    });
    await compliance.register(pks);

    // find related key (-x, y)
    const PRIME =
        21888242871839275222246405745257275088548364400416034343698204186575808495617n;
    const ORDER =
        21888242871839275222246405745257275088614511777268538073601725287587578984328n;
    const relatedX = PRIME - user.publicKey[0];
    const realPriv = await crypto.formatPrivateKeyForBabyJub(user.privateKey);
    const relatedPriv = ORDER - realPriv;
    const relatedUser = {
        privateKey: relatedPriv,
        publicKey: [relatedX, user.publicKey[1]] as Point,
    };

    // share secret
    let { input, proofPayload } = await makeShareSecretParams(
        user,
        compliance,
        crypto,
        false // hack
    );

    // proof contains the normal user key (x, y), 
    // but public key (which contributes to the AML pubkey) is (-x, y)
    await expect(
        compliance.shareSecret(input, proofPayload, [
            relatedUser.publicKey[0],
            relatedUser.publicKey[1],
        ])
    ).to.be.reverted;
});

with the following modifications in packages/crypto/src/secretSharing.ts to allow direct use of the private key:

async function secretSharing(
   secretSharingArtifacts: Artifacts,
   groth16: any,
   utils: Utils,
+  hack?: boolean,
 ): Promise<SecretSharingProof> {
+  let realPrivKey = privateKey;
+  if (!hack) {
+    realPrivKey = await utils.formatPrivateKeyForBabyJub(privateKey);
+  }
+
   // construct the polynomial for the privateKey
   indexes.forEach((el) => assert(el !== 0n)), 'invalid index';
-  const polynomial = constructPolynomial(threshold,
-    await utils.formatPrivateKeyForBabyJub(privateKey),
-    utils,
-  );
+  const polynomial = constructPolynomial(threshold, realPrivKey, utils);

and the following changes in packages/contracts/test/compliance.test.ts to enable the tweak:

-const makeShareSecretParams = async (sender: KeyPair, compliance: any, crypto: Utils, index?: bigint) => {
+const makeShareSecretParams = async (sender: KeyPair, compliance: any, crypto: Utils, hack?: boolean, index?: bigint) => {

Recommendation. We recommend avoiding using an ambiguous representation of the public key in general. Either use the compressed format (which packs the sign of $x$ in the MSB of $y$), or use the uncompressed format.

Description. There are several implied state transitions in the compliance contract. For example, the contract could have been uninitialized (with an identity AML public key $(0,1)$), or initialized but with not enough peers (due to the threshold set at 5), or initialized with enough peers but new members are still being added, and so on.

Setup flow. In the setup flow, new public keys can be added by the owner of the smart contract:

1. The function register() takes an arbitrary-length array of public keys (as well as an index for each public key, used by the secret sharing scheme).
2. When everyone is registered (by potentially multiple calls to register()), then each committee member should call shareSecret(). This will split their own secret between everyone else (in a verifiable way) and add their contribution to the AML public key.
3. In the case were the number of registered keys is greater than SHARE_SIZE (currently set at 11), then the previous call to shareSecret() only encrypted shares to some of the participants, and thus calls to shareSecretForOne() are necessary to distribute shares to the rest of the committee.

None of the calls described are being tracked. It is thus possible for the AML public key to be used before everyone has had a chance to share their secret. It is also possible that the committee won't be large enough compared to the threshold (currently set at 5), or that some members won't have received everyone's shares and thus will have trouble contributing in threshold decrypting transactions that happened during that period.

New member flow. New members can be added via the following steps:

1. New keys can be registered by the smart contract owner at any point in time.
2. A new member willing to join the committee will have to call shareSecret() to share their own secret with the current committee. Potentially followed by several calls to shareSecretForOne().
3. Then, the rest of the committee will have to call shareSecretForOne() to make sure that the new member also has everyone's shares.

Here again, the new AML public key (after step 2) can be used in spite of step 3 not having been executed (or in spite of the new member not having distributed shares to everyone).

Removing a member flow. Members can be removed from the committee via the following steps:

1. The smart contract owner can remove the member by calling remove(). This will remove their share in the AML public key.

After executing this step, the committee might be under the THRESHOLD (currently set at 5) and thus incapable of decrypting transactions until a new member is added.

Furthermore, the member that was excluded still has access to the shares of others. With enough collusion between dishonest or/and excluded members (assuming that members get kicked out of the committee for acting maliciously), it might be possible for them to recover the AML private key.

Pending keys. In the previous flows, we omitted an important aspect of the compliance smart contract: if there are too many registered keys, public keys get added to an array of pending keys. This notion of pending keys impacts all aspects of the state transitions. For example, when a registered key is removed, it is replaced with a pending key (in FIFO order).

It is not clear how the smart contract owner should deal with pending keys. For example, If the maximum number of committee members is increased (via increasing _maxRegisteredKeys) how can the owner transition pending keys to newly available seats in the committee? It seems like the only way is to call remove() on a register key, which might have unwanted additional effects. Another example, what if the owner decreased _maxRegisteredKeys instead, and then needed to remove some committee members? Calling remove() will actually not remove() a member, it will replace them with the first pending user.

Recommendation. Now that we have stated the different issues with the lack of explicit encoding of the state transitions in the smart contract, we discuss some possible solutions:

• Add a global boolean that indicates if the contract has been initialized. make sure that the SMASP contract cannot use an uninitialized contract by checking that boolean. Have the owner manually flip that boolean, or have it be flipped automatically when enough keys have been registered and shared.
• Think about removing the notion of pending keys. It might be an unnecessary feature that adds complexity and more surface area for bugs.
• Think about ways to rotate shares when members are being removed for malicious behavior. Rotating shares could either be done by prompting current members from rejoining under a different keypair, or by including an epoch number that increases when members leave and prompts a new resharing.

Description. A user attempting to join the compliance committee is expected not to call shareSecret() several times. This is checked in the shareSecret() function by ensuring that the $y$-coordinate of the commitment to the first coefficient of the user's polynomial (the user key) is $0$:

require(yPolynomialPoints[yPublicKey][0] == 0, "Already shared secret");

It is not clear at present, if this check is impossible to pass for a malicious user. If it is possible, then the user could join the committee several times. While this would not allow them to contribute to the AML public key several times (due to an additional check with isPublicKeyAdded), this will emit several events with potentially different polynomial commitments and encrypted shares. This, in turn, could potentially muddle the threshold decryption of transactions later on.

Passing this check seems possible if either the malicious user can find the discrete logarithm of one of the two points with coordinate $y=0$, which is highly unlikely, or if the circuit can interpret the point $(0,0)$ as the point at infinity, which is more likely.

For example, the solidity implementation of the Baby Jubjub curve used currently handles $(0,0)$ as the point-at-infinity (likely for compatibility with applications that encode the identity as $(0,0)$). See CurveBabyJubJub.sol:

function pointAdd(uint256 _x1, uint256 _y1, uint256 _x2, uint256 _y2) internal view returns (uint256 x3, uint256 y3) {
    if (_x1 == 0 && _y1 == 0) {
        return (_x2, _y2);
    }

    if (_x2 == 0 && _y1 == 0) {
        return (_x1, _y1);
    }

We were not able to ensure that this was not the case as the circuits were not shared with the consultants. If it is the case, the malicious user would simply have to use one of the valid points that has $y=0$ (which is easy to compute), and then submit a proof that uses the secret share $0$.

A secret share of $0$ would mean that the first coefficient of the user's polynomial is $0$, which in turn would mean that the commitment to that coefficient would be the point at infinity. If that point at infinity can be encoded as $(0,0)$ then it will match the registered key's $y$ coordinate as well as the check mentioned above.

Recommendation. As discussed in finding #2, do not use an ambiguous way to refer to a public key. Addressing a public key with its uncompressed coordinates or its compressed coordinates is non-ambiguous, while using only the y-coordinate is ambiguous.

Additionally, use stricter methods to check if a member has already called shareSecret(). For example, by using a map from public keys to a boolean in the global state.

Description. Members of the compliance committee are registered through the register function of the compliance contract. (Note that only the contract owner can call the register function.) For each public key being registered, an index is also associated with it. This is necessary for the distributed key generation to work, as each member needs to receive evaluations at the same point (or index).

function register(PublicKey[] memory pk) public onlyOwner {
    // TRUNCATED...

    indexes[pk[i].yPublicKey] = pk[i].index;

The problem is that the owner might use the same index twice, which would result in a smaller committee than expected. Two members might act as one, in some sort.

Recommendation. It might be a good idea to use a counter to compute the next index, and increment it as part of the logic, or check that the index is not already used during registration.

It also seems like an acceptable risk if the owner is careful enough.

Description. In /packages/contracts/hardhat.config.ts the version of the Solidity compiler to use in the project is fixed:

solidity: {
    version: "0.8.9",

The latest version of the Solidity compiler at the time of this writing is 0.8.20 (https://github.com/ethereum/solidity/releases), which was released on May 10, 2023. The version currently used by Silent Protocol is from September 29th, 2021 (https://github.com/ethereum/solidity/releases/tag/v0.8.9), which is severely outdated.

Recommendation. As there's been a number of bug fixes since version 0.8.9, we recommend Silent Protocol to upgrade the version of the Solidity compiler to the most recent one.

Description. Silent core makes use of the uniswap contract UniswapV2Router01 in order to calculate user fees in Silent token.

The Uniswap documentation warns projects to migrate to the new version of the contract:

UniswapV2Router01 should not be used any longer, because of the discovery of a low severity bug and the fact that some methods do not work with tokens that take fees on transfer. The current recommendation is to use UniswapV2Router02.

Recommendation. While the bug affecting the version of the dependency used doesn't seem to affect Silent Core's use of the smart contract, the mention that "some methods do not work with tokens that take fees on transfer" might be enough of a reason to upgrade the dependency.

Description. The remove function of the compliance smart contract does not explicitely handle the case where the given public key is not registered (and thus cannot be removed).

While we did not find ways to exploit this (the execution reverts later), it seems like the code forgets to explicitely check that the public key is registered before trying to remove it.

The function with added comments:

function remove(uint256[2] memory publicKey) public onlyOwner {
    // this will return 255 if empty
    uint8 index = getRegistrationIndexOfPublicKey(publicKey[1]);

    uint256 _firstPendingKeyIndex = firstPendingKeyIndex;

    if (pendingKeys.length > _firstPendingKeyIndex) {
        // this will error as index is out of bound
        registeredKeys[index] = pendingKeys[_firstPendingKeyIndex];

        // TRUNCATED...
    } else {
        // this will error as index is out of bound
        registeredKeys[index] = registeredKeys[registeredKeys.length - 1];

        // TRUNCATED...
    }

    // TRUNCATED...
}

Recommendation. We recommend adding an explicit error at the top of the function in case getRegistrationIndexOfPublicKey returns the value 255 (which stands for unregistered public key).