Datapunk

the best practice for updateGroupAdmin is through a two-step update: existing admin assign the new potential admin to a new address the new address accept the admin role. This is...

a gas optimization: in functions like addMember, we currently have uint256 merkleTreeRoot = getMerkleTreeRoot(groupId);, while merkleTreeRoot could have been returned from _addMember call https://github.com/semaphore-protocol/semaphore/blob/8eb19e83fda62644872b2fcfbd85011d3b2c21e2/packages/contracts/contracts/base/SemaphoreGroups.sol#L70

the _addMember function is missing `onlyGroupAdmin(groupId)` check, Without it anyone can add to the group. https://github.com/semaphore-protocol/semaphore/blob/8eb19e83fda62644872b2fcfbd85011d3b2c21e2/packages/contracts/contracts/base/SemaphoreGroups.sol#L83

define a const variable for max_depth

1

It is better to define a const for 12 https://github.com/semaphore-protocol/semaphore/blob/8eb19e83fda62644872b2fcfbd85011d3b2c21e2/packages/contracts/contracts/Semaphore.sol#L139

A malicious actor can monitor calls of createGroup(groupId, admin) and frontrun it with createGroup(groupId, admin1). This prevents new group's creation. If we adopt an incremental groupId, once groupId increases by...

verifyProof probably should return false for any key if the tree is empty

3

https://github.com/privacy-scaling-explorations/zk-kit/blob/253277c91c8ffee33bfb4bc5abd8a0db0251df61/packages/smt/src/smt.ts#L237 not sure if it is as designed, but verifyProof return True for any key with an empty tree. It makes better sense if false is returned: ``` verifyProof(merkleProof: MerkleProof):...

the type of childNodes is not comprehensive

1

https://github.com/privacy-scaling-explorations/zk-kit/blob/de43718b9a400f46b3b89a288d01b9d32e9b17d3/packages/smt/src/smt.ts#L292 here childNodes could be a leaf node of Entry type ([Key, Value, EntryMark]). However, the type system is defined as: private nodes: Map export type ChildNodes = Node[] export...

use of BigInt is vulnerable to timing attacks

1

https://github.com/privacy-scaling-explorations/zk-kit/blob/c912111c134b6bbefe223bb61ba22f864036e9ba/packages/poseidon-cipher/src/poseidonCipher.ts#L19 implementation that uses the javascript BigInt is vulnerable to timing attacks ([more info](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt#cryptography)). If constant-time operation is a requirement, this needs changes to use constant time modular math libraries....

checking if newLeaf != this._nodes[0][index] first before update

1

https://github.com/privacy-scaling-explorations/zk-kit/blob/c912111c134b6bbefe223bb61ba22f864036e9ba/packages/imt/src/imt.ts#L227 we can save some computation by checking if newLeaf != this._nodes[0][index] first?

https://github.com/privacy-scaling-explorations/zk-kit/blob/c912111c134b6bbefe223bb61ba22f864036e9ba/packages/imt/src/types/index.ts#L4 for JS/imt, it is clearer to define export type IMTNode = number | string | bigint; directly istead of any. All functions require such types anyway