Audit of Smart Contract for On-chain Stamp Integration

[erichfi]

The primary goal of this SPIKE is to conduct a thorough audit of the smart contract responsible for bringing stamps on-chain. This is a crucial component of our system, and as such, requires a meticulous review to ensure its security, efficiency, and compatibility with the rest of our infrastructure.

Tasks:

• [x] Code Review: Deep-dive into the smart contract code to identify any potential bugs, security vulnerabilities, or inefficiencies. Look out for standard code smells and areas where the contract could be optimized.
• [x] Functional Testing: Test the smart contract in various scenarios to ensure it behaves as expected. This should include both typical use cases and edge cases to understand how the contract handles unexpected or invalid inputs.
• [x] Security Analysis: Assess the smart contract for common security vulnerabilities, such as re-entrancy attacks, integer overflow/underflow, front-running, etc. Employ standard security analysis tools where applicable.
• [x] Gas Analysis: Evaluate the gas costs associated with executing different parts of the contract. Identify areas where gas usage can be optimized.
• [x] Interoperability Testing: Test the smart contract's interoperability with other parts of our system, including the Verifier and Attester contracts, and the Passport app.
• [x] Review Attester & Verifier Contracts: Given their close interaction with the smart contract in question, these contracts should also be reviewed to ensure they do not introduce vulnerabilities or inefficiencies.
• [x] Assess Upgradability and Future-proofing: Evaluate the contract’s ability to be upgraded or modified in the future without disrupting its existing functionality.

Deliverables:

• Audit Report: A comprehensive report detailing all findings from the audit, including any vulnerabilities or inefficiencies identified and their proposed solutions.
• Recommendations: A list of recommended changes or improvements to the smart contract, ranked by priority.
• Future-proofing Guidelines: Suggestions on how to ensure that the smart contract remains secure and efficient as the system evolves.

Once we've completed this SPIKE, we will be well-equipped to make informed decisions about any necessary changes or improvements to the smart contract, bolstering the security and efficiency of our on-chain stamp integration.

[tim-schultz]

Testing

• [ ] Make unit tests as easy to understand as possible. Remove duplicate mocks and comments where necessary
• [ ] Ensure final contracts have 100% test coverage. As of 5/18 we are at 91.3%
• [ ] Write fuzz tests? - It looks like https://github.com/crytic/echidna is a popular choice

Both Contracts

• [ ] Lock the solidity version

GitcoinAttester

• [ ] remove setEASAddress and set address in constructor
• [ ] eas address is redundant if we have EAS contract we can get address

GitcoinVerifier

• [ ] revert within the _verify function. With this method you could remove the if statement all together and revert for the independent reasons(nonce/signature)
• [ ] remove _hashArray
• [ ] DOMAIN_SEPARATOR (contracts/GitcoinVerifier.sol#22) should be immutable
• [ ] attester (contracts/GitcoinVerifier.sol#16) should be immutable
• [ ] issuer (contracts/GitcoinVerifier.sol#17) should be immutable
• [ ] bytes32 private constant EIP712DOMAIN_TYPEHASH, bytes32 private constant STAMP_TYPEHASH, bytes32 private constant PASSPORT_TYPEHASH can be prehashed and the values can be assigned to the bytes32 value - will minimize gas in deployment
• [ ] Add an onlyOwner withdraw function to contract. Funds would currently be locked forever. This would also require the contract to be updated to ownable. Another option could be an allowed address set in the constructor/deployment
• [ ] Convert loops to unchecked loop to reduce gas usage https://hackmd.io/@totomanov/gas-optimization-loops#2-Increment-the-variable-in-an-unchecked-block

General

• [ ] remove console.logs
• [ ] Adjust hardhat compiler version to match solidity version that we lock to
• [ ] Move to yarn? We use this for all other project
• [ ] Ensure all contracts are fully commented
• [ ] Improve error handling - revert should be specific to case that causes revert
• [ ] Deployment Checklist(set env variables, contract owners, verifiers, etc..)

[lucianHymer]

To save some gas, we could make the iterator in the for loop of _hashPassport unchecked. Nobody would ever have > 2^256 stamps so we shouldn't have to worry about overflow, and even if it did overflow they'd just fail validation b/c it wouldn't match.

I think we can also just do uint256 i instead of uint256 i = 0, in solidity it starts as 0.

[lucianHymer]

In _verify, we should do the revert statements as Tim mentioned. We can drop the if statement both in Attester waiting on the verify result, and the if statement around recipientNonces[]++. Also, no need for verify to return a value.

In hashPassport, passport should be passed as calldata

We should revert with custom errors instead of strings.

Also, my previous comment applies to the for loop in getMultiAttestRequest as well.

Here's a PR where I tried some of this stuff out (didn't add the custom errors to this): https://github.com/gitcoinco/eas-proxy/pull/13

[nutrina]

Have merged in https://github.com/gitcoinco/eas-proxy/pull/13 , this looks good.