Hinkal security record

The report gives no description here.

Relayer fees are calculated by a percentage of the exchanged token amount without considering the actual gas usage of the transaction. When making a `transact()` call on Hinkal, users can specify hook contracts, which `Hinkal` will call specific functions on them before or after the token exchange. A user may specify their hook contracts and spend a large amount of gas during the hook function calls, theoretically up to the block gas limit. As a result, the relayer may spend a large amount of gas executing the user's transaction but does not receive enough fees in return. The support of user-specified hooks means external calls can be performed to arbitrary addresses for each transaction. This can result in unexpected behavior, like the aforementioned gas griefing. We advise the Hinkal team to limit such capabilities to reduce the attack surface of the protocol.

As the merkle tree is constructed, there are times when a node is hashed with another node representing an empty portion of the tree. `MerkleRemoveable` does this when it indexes into the `tree[]` for a given index for the first time: ``` // insert() for (uint256 i = 1; i <= twoPower; i++) { currentNodeIndex /= 2; tree[currentNodeIndex] = hash( tree[currentNodeIndex * 2], tree[currentNodeIndex * 2 + 1] ); } ``` `Merkle` does this for all left nodes: ``` // insertOne() for (uint256 i = 0; i <= twoPower; i++) { if (currentNodeIndex % 2 == 0) { tree[i] = prevHash; if (i != twoPower) prevHash = hash(prevHash, 0); } else { prevHash = hash(tree[i], prevHash); } currentNodeIndex /= 2; } ``` However, the empty nodes are incorrectly represented as `0`. Leaves should be represented to be `hash(0)`, the layer above to be `hash(hash(0), hash(0))`, and so on to conform to the standard definition of a Merkle tree. Hashing with `0` rather than the correct degree of hashed zeroes will mean that the tree contains valid preimages that do not map to any real transaction. If such preimages are found, they may be exploited.

There's a known `Groth16` malleability vulnerability that can lead to generating two valid proofs for the same set of inputs for a replay attack. The Hinkal protocol keeps a mapping of nullifiers to prevent replay attacks by re-using them. However, due to Malleability, generating another valid proof for the same inputs for the commitment is possible. This allows every commitment to be used twice. Here is a comprehensive explanation of [Tornado Cash vulnerability](https://medium.com/@Beosin_com/exploring-tornado-cash-in-depth-to-reveal-malleability-attacks-in-zkp-projects-f7321922f82d).

The `calldataHash` field is unconstrained in Circom and not used at all. However, before we call the verifier, the Hinkal checks in `HinkalHelper.sol` make sure that (a part) of the Circom data matches the provided hash. The `calldataHash` represents information that is not a part of the proof calculation. So, in essence, via the hash, the user/relayer commits to the information that is not checked by Circom, so that the proof cannot be used with other external actions and hooks. Now we have the following scenarios of what data the user can submit to the transaction: 1. **A hash that has the integrity with the data, and was used to generate the proof.** This is a legit scenario that passes both the helper check, and the proof verification. It works as expected. 2. **A hash that does not have the integrity with the data, but was used to generate the proof.** This is going to be rejected by the helper. It works as expected. 3. **A hash that does not have the integrity with the data, and was NOT used to generate the proof, though the proof is another passing proof (it was generated using a different hash).** Such a transaction would be rejected by the helper check, as expected. 4. **A hash that has the integrity with the data, and was NOT used to generate the proof, though the proof is another passing proof (it was generated using a different hash).** This check is going to pass the verifier, because the proof is valid for some other hash, and it is also going to pass the calldata check in `HinkalHelper.sol` as there is integrity with the calldata that was also spoofed and is not checked in the proof. <br> **This opens an opportunity for a malicious relayer or a front-runner to add external action calls and hooks that the user did not intend, as well as redirect funds to a different `externalAddress`. The impact of these calls will depend on the logic encoded in the respective contracts, hence we classify the severity of this vulnerability as undetermined.** During our testing, we found that unused public parameters in circom still led to unique proof generation. However, [in some compiler versions](https://github.com/iden3/circom/releases/tag/v2.0.4), unconstrained/unused public parameters are optimized out. If this is ever the case, now or in the future, the soundness of the zero-knowledge mechanism will break, and the attack vector will be reintroduced. This can lead to a high-severity impact on the protocol.