Hinkal security record

According to [Circom's documentation](https://docs.circom.io/circom-language/basic-operators/#arithmetic-operators), the arithmetic operations in the circuits are calculated modulo `CIRCOM_P`. The swapper circuits (`SwapperM`, `SwapperMO`, and `SwapperToken`) calculate the input amount and output amount sums for each token and check whether the difference is the expected amount change. Because the addition operations work modulo `CIRCOM_P`, it is possible for the result to overflow when adding up the sum of input or output amounts. As a result, an attacker could forge some output commitments with a large token amount but whose sum results in a small number. They can then drain the funds in the protocol with the forged commitments. This attack does not exist for the currently implemented Swapper circuits, but it may be introduced with new circuits if they use vulnerable constructor arguments.

Contracts inherit the contract `Ownable`, where the owner role can be transferred to another address simply by calling the function `transferOwnership()`. This function immediately transfers a high-level privilege to new addresses in a single transaction, which can be risky from a security perspective, as providing a faulty address may lock out that role from future calls. A more secure pattern for such privilege transfers is to require the new pending addresses to issue an `acceptAdmin()` function call before finalizing the transfer. Note that this pattern is common even with the use of timelocks, such as the Compound Timelock contract.

The deletion of an access token via the `AccessToken.blacklistAccessKey()` function removes a leaf node from the `MerkleRemovable` tree, requiring to update of the values for all parent nodes, including the root. As a necessary measure, all the cached roots need to get removed to assure that the blacklisted access key can no longer use the protocol. However, this will cause all pending Hinkal transactions by legitimate users to fail, as the `HinkalHelper.performHinkalChecks()` will revert, as the `rootHashAccessToken` those transactions used to construct the proofs are no longer valid.

The purpose of the contract `Hinkal` is to receive deposits from external actors. Tokens should be deposited into the contract by whitelisted actors via a specific flow and the execution of the function `Hinkal._transact()`. However, it is also possible to send these tokens via a direct token transfer. If the logic of the contract is not clear for an external user of the system, they may deposit tokens via a direct transfer. In that case, the amount would not be accounted for by the contract and remain stuck in the contract.

When a transaction is submitted to Hinkal, the function `HinkalHelper.relayerIsValid()` requires that the relayer is whitelisted. However, the function checks the value against `tx.origin`. This has two consequences: 1. A relayer cannot be a smart contract. 2. If the transactions initiated by a given relayer are not restricted to relaying Hinkal transactions, the relayer can initiate a transaction that contains the execution of a malicious contract. In this case, the malicious contract can trigger an external call to `Hinkal.transact()` with the address of the relayer in `circomData`. The impact is limited here because the rest of the data in `circomData` should be accepted by the system and may likely result in a reverted transaction.

The system supports transfers of `ERC20` and `ERC721` tokens. Several contracts distinguish if the expected token to be transferred is an `ERC20` or `ERC721` based on the parameter `tokenId`: * `== 0` will be considered as an `ERC20` transfer. * `> 0` will be considered as an `ERC721` transfer of that specific `tokenId`. However, as mentioned in the `ERC721` [standard](https://eips.ethereum.org/EIPS/eip-721), a `tokenId` of value `0` would be valid: > Every NFT is identified by a unique uint256 ID inside the ERC721 smart contract. This identifying number SHALL NOT change for the life of the contract. The pair (contract address, uint256 tokenId) will then be a globally unique and fully qualified identifier for a specific asset on an Ethereum chain. While some ERC-721 smart contracts may find it convenient to start with ID 0 and simply increment by one for each new NFT, callers SHALL NOT assume that ID numbers have any specific pattern to them, and MUST treat the ID as a “black box”. As a result, any attempt to operate with an `ERC721` token having the id `0` may revert or result in an unexpected output since the token will be incorrectly considered as an `ERC20` instead of an `ERC721`. Impacted functions include: * `CircomDataBuilder.formInputForCircom()` where the function `formInputNormal()` will be called instead of the function `formInputNft()`. * `Hinkal._internalTransact()` where the function `transferERC20TokenOrETH()` will be called to pay a relayer fee and will most likely revert. * `Transferer.getBalancesForArray()` and `Transferer.getBalancesForArrayMemory()`, where the function `getERC20OrETHBalance()` will be called instead of `getNftBalance()`. * `Transferer.transferTokenFrom()`, where the function `transferERC20TokenFromOrCheckETH()` will be called instead of `transferNftFrom()`. * `Transferer.transferToken()`, where the function `transferERC20TokenOrETH()` will be called instead of `transferNftFrom()`. * `Transferer.approveToken()`, where the function `unsafeApproveERC20Token()` will be called instead of `approveERC721Token()`.

The Hinkal protocol implements a whitelist mechanism for users, where users have to obtain an access token via the KYC process and add the token to the registry before using the protocol. Access keys are calculated by `hash(shieldedPrivateKey, publicKey)` and are stored as the leaves of the `AccessToken` Merkle tree. The `AccessTokenChecker` circuit is to ensure that (1) a user's access key is in the Merkle tree and (2) they know some `X` and `Y` such that `hash(X, Y)` equals to their access key. Since the entire `AccessToken` Merkle tree is public on-chain, an attacker can generate a ZK proof to prove an internal node of the Merkle tree is in the tree by setting `X` and `Y` to its left and right child. The condition of `hash(X, Y)` being the internal node holds naturally. The attacker does not have to obtain an access token in the first place to generate a valid proof. Notice that this issue may not currently impact the protocol given that the swapper circuits calculate a user's `publicKey` as the hash of their `shieldedPrivateKey`. However, the design of the `AccessTokenChecker` circuit poses a security risk and could be vulnerable if used separately.

In addition to native tokens, the protocol also enables arbitrary ERC-20 tokens to be interacted with in the protocol. The protocol includes an `ERC20TokenRegistry` contract that can theoretically maintain a list of whitelisted ERC-20 tokens, but the auditors were informed that this contract will be disabled and not used. Enabling the support for arbitrary ERC-20 tokens is always a challenge. Malicious tokens can break assumptions about standard functions such as `balanceOf()` and others. It could grant certain addresses infinite approval to transfer tokens back from Hinkal, make them non-withdrawable from the protocol, and many other things. For such malicious token implementations, the UTXOs within Hinkal, or in other words the commitments in the Merkle tree, would not necessarily match the balance of Hinkal. While these tokens most likely would not be of any value, we still wanted to raise this as an informational issue.

It is important to validate inputs, even if they only come from trusted addresses, to avoid human error: * `AccessToken.constructor()`: Set a maximum limit for the value of `_mintingFee`. * `AccessToken.setMintingFee()`: Set a maximum limit for the value of `_mintingFee`. * `ERC20TokenRegistry.contructor()`: Validate that `intialTokenSet[]` doesn't contain the zero address. * `ERC20TokenRegistry.addERCToken()`: Validate that `erc20Token` is not the zero address. * `Hinkal.registerExternalAction()`: Validate that `externalActionId` and `externalActionAddress` are both non-zero. * `Hinkal.transactWithExternalActionAndHook()`: Validate that `circomData.externalAddress` is not the zero address. * `HinkalHelper.checkTokenRegistry()`: Validate that the input arrays are the same length. * `HinkalHelper.performHinkalChecks()`: * Validate that `circomData.erc20TokenAddresses.length == circomData.amountChanges.length == circomData.tokenIds.length`. * Validate that for all `i` with `circomData.tokenIds[i] != 0`that `amountChanged[i]` is equal to one or minus one. * `RelayStore.addOrSetRelay()`: * Validate that the `_relayAddress` is not the zero address. * Validate that the `_url` is non-empty. * `Transferer.multiTransfer()`: Validate that the input arrays are the same length. * `Transferer.multiTransferFrom()`: Validate that the input arrays are the same length. * `Transferer.getBalancesForArrayMemory()`: Validate that the input arrays are the same length. * `Transferer.getBalancesForArray()`: Validate that the input arrays are the same length. * `Transferer`: All transfer functions should require transferring a non-zero amount. * `Merkle.insertMany()`: Validate that the input array `leaves` is non-empty. Otherwise, it could result in a copy of the value `roots[rootIndex]` in `roots[(rootIndex+1) % MAX_ROOT_NUMBER ]`. This could ultimately become a way to limit partially DoS the system by making sure that the history of the last `MAX_ROOT_NUMBER` valid roots contains the same value. * `MerkleBase.constructor()`: * Validate that `poseidon` is a non-zero address. * Valdiate that `LEVELS` is initialized to be less than 128. * `MerkleBase.rootHashExists()`: Validate that `_root` is non-zero. * `VerifierFacade.registerVerifier()`: * Validate that `verifierMap[verifierId] == 0` before the update. * Validate that `verifierAddress` and `verifierId` are both non-zero. <br> **Circom Circuits:** There are no guarantees that the input values to parameters like addresses are valid. For example, if the `SwapperToken` circuit is used and the wrong `nftPosition` is specified, some NFTs may be treated as fungible tokens. Update `P` and check that variables for all fields have valid integer interpretations.

Hinkal transactions can optionally be executed with: 1. A pre-transaction hook which calls a contract at the address `circomData.preHookContract` with the calldata `circomData.hookMetadata`. 2. A post-transaction hook which calls a contract at the address `circomData.hookContract` with the calldata `circomData.hookMetadata`. If a user wants to execute both a pre-transaction hook and a post-transaction hook in the same transaction, the hooks will have to use the same `circomData.hookMetadata`. As a result, specific customization will be required on each of these contracts to parse the received calldata and cover both cases: * The contract is called without the other hook. * The contract is called in addition to the other hook.