From 73a1faf148715a13536ab5ccee6fad8648cee9e5 Mon Sep 17 00:00:00 2001 From: Aditya Anand M C Date: Fri, 20 Oct 2023 14:32:17 +0530 Subject: [PATCH 1/6] chore: first pass --- docs/00-onchain-data.md | 9 ++++----- docs/01-onchain-passport-attestation.md | 10 +++++----- review_aditya.md | 21 +++++++++++++++++++++ 3 files changed, 30 insertions(+), 10 deletions(-) create mode 100644 review_aditya.md diff --git a/docs/00-onchain-data.md b/docs/00-onchain-data.md index 118df75..5f61505 100644 --- a/docs/00-onchain-data.md +++ b/docs/00-onchain-data.md @@ -14,7 +14,7 @@ How does this work? 2. Once the schema was created, you can write data to it by calling one of the EAS smart contracts functions, for example `attest(AttestationRequest calldata request)` - (see [IEAS.sol](https://github.com/ethereum-attestation-service/eas-contracts/blob/master/contracts/IEAS.sol#L148-L169)) + (see [IEAS.sol](https://github.com/ethereum-attestation-service/eas-contracts/blob/c4c63775094e9104d0b4335ac8428f8d10f6d589/contracts/IEAS.sol#L116-L132)) 3. The following data will be registered in the attestation: 1. the attester (this will be the`msg.sender`) 2. the recipient (an ETH address) @@ -177,10 +177,9 @@ mapping(address => mapping(bytes32 => bytes32)) public userAttestations; ``` In order to ensure the integrity of the data that a resolver stores, resolver -smart contract shall only validate and store date from trusted sources: - -- a trusted EAS contract -- a trusted Attester +smart contract shall +- only allow calls from a trusted EAS smart contract +- only accept data coming from a trusted attester ## GitcoinPassportDecoder diff --git a/docs/01-onchain-passport-attestation.md b/docs/01-onchain-passport-attestation.md index 4f59f99..9e053ac 100644 --- a/docs/01-onchain-passport-attestation.md +++ b/docs/01-onchain-passport-attestation.md @@ -61,9 +61,6 @@ new SchemaEncoder( hashes field, this is an ordered array, containing th expiration date for each stamp. -Considering the list of providers above if a user has the `BrightId`, -`CommunityStakingSilver` and `Discord` stamps, their attestation will look like: - #### providerMapVersion - this field will be used to indicate the version of the provider map used to @@ -71,10 +68,13 @@ Considering the list of providers above if a user has the `BrightId`, if there are a large amount of stamps that need to be replaced/removed in the future. If new providers are only added, the providerMapVersion does not change. +Considering the list of providers above if a user has the `BrightId`, +`CommunityStakingSilver` and `Discord` stamps, their attestation will look like: + ```json { - "providers": ["12983612785124"], - "hashes": ["0x0000000000000001", "0x0000000000000002", "0x0000000000000003"], + "providers": ["0x0000000000000029"], // 01 + 08 + 20 + "hashes": ["0x0000000000000001", "0x0000000000000008", "0x0000000000000020"], // [BrightId, CommunityStakingSilver, Discord] "issuanceDates": ["123456789", "123456789", "123456789"], "expirationDates": ["123456789", "123456789", "123456789"] } diff --git a/review_aditya.md b/review_aditya.md new file mode 100644 index 0000000..242481b --- /dev/null +++ b/review_aditya.md @@ -0,0 +1,21 @@ +# GitcoinVerifier + +**Questions** +- `L282`: Thoughts on just withdrawing contract balance as opposed to specifying an amount ? + +**Low** +- `L6` : Recommend using `@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol` +- `L43-L80`: Recommend keeping constants with storage variables +- `L94`: Recommend adding natspec for `__GitcoinVerifier_initno no` +- `L34`: can be made constant with value `GitcoinVerifier`. Remove `L100` during initialize +- `L235`: Consider addressing the TODO via loop. It does increase gas cost but likely worth it unless passport team is confident that the IAM will work as expected +- `L202`: Recommend storing the length in variable as opposed to computing it at every iteration. Gas Optimisation +```javascript +uint256 attestationsLength = attestationRequest.multiAttestationRequest.length; +bytes32[] memory multiAttestHashes = new bytes32[](attestationsLength); +// avoids computing attestationsLength on every iteration +for (uint i = 0; i < attestationsLength; ) { + ... +} +``` + From e581a6632daf249a2a58382e4f879be2a61e0a4e Mon Sep 17 00:00:00 2001 From: Aditya Anand M C Date: Mon, 30 Oct 2023 19:01:42 +0530 Subject: [PATCH 2/6] second pass --- review_aditya.md | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/review_aditya.md b/review_aditya.md index 242481b..b3c985c 100644 --- a/review_aditya.md +++ b/review_aditya.md @@ -1,7 +1,11 @@ +**General** +- Improve natspec documentation to capture input params and return value + # GitcoinVerifier **Questions** - `L282`: Thoughts on just withdrawing contract balance as opposed to specifying an amount ? +- `L182`: When encoding, why include MULTI_ATTESTATION_REQUEST_TYPEHASH ? Is it cause passport aims to supports another verifier which could use a different format ? **Low** - `L6` : Recommend using `@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol` @@ -19,3 +23,43 @@ for (uint i = 0; i < attestationsLength; ) { } ``` +# GitcoinAttester + +**Questions** +- `L67`: Would it be possible to make `setEASAddress` external ? + +**Low** +- `L5` : Recommend using `@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol` +- `L20`: Mark the visibility of IEAS as `public` / `private` and then add a external function to read that +- `L48`, `L59`: Recommend adding custom errors like `VERIFIED_ALREADY_ADDED` / `INVALID_VERIFIED` for gas optimization +- `L68`: Emit event to make it easier to index and have the latest EAS address. Also add zero address check. Why not set this during `initialize` as well +- `L80`/ `L93` The msg.sender == owner() feels like extra code. Would recommend setting owner as verifier during `initialize` and clean update the check to. Also use custom errors instead of `require` +``` if (!verifiers[msg.sender]) revert NOT_VALID_VERIFIER() +``` + +# GitcoinResolver + +**Questions** + +- Once the flattened attestations are retured from EAS -> GitcoinAttestor -> GitcoinVerifier. How does the Resolver pick it up ? Is it a hook from EAS which invokes `attest` ? + +**Low** +- `L59` Update check to ensure _gitcoinAttester is not zero +- `L135` Recommend gas optimizing +``` +uint256 attestationsLength = attestations.length +for (uint i = 0; i < attestationsLength; ) { + ... +} +``` + +# GitcoinPassportDecoder + +**Low** +- `L20`: Mark the visibility of IEAS as `public` / `private` and then add a external function to read that +- `L58`: Emit event `setEASAddress`. Add zero check +- `L67`: Emit event `setGitcoinResolver`. Add zero check +- `L74`: Emit event `setSchemaUID`. Add zero check +- `L82`: Emit event `addProvider`. Add zero check +- `L90`: Emit event `createNewVersion`. +- `L146`, `L147`: Store .length as variables as use them in the for loop to save gas \ No newline at end of file From 4c8ebb2cf21742472fa5fe52c488ffa189aef593 Mon Sep 17 00:00:00 2001 From: Aditya Anand M C Date: Mon, 30 Oct 2023 19:13:51 +0530 Subject: [PATCH 3/6] third pass --- review_aditya.md | 1 + 1 file changed, 1 insertion(+) diff --git a/review_aditya.md b/review_aditya.md index b3c985c..9934be0 100644 --- a/review_aditya.md +++ b/review_aditya.md @@ -1,5 +1,6 @@ **General** - Improve natspec documentation to capture input params and return value +- Nitpick but you could explore moving all the interfaces to it's own folder # GitcoinVerifier From e937124392d278aeb1bf86e9c92cb49c73fe68c7 Mon Sep 17 00:00:00 2001 From: 0xKurt Date: Mon, 30 Oct 2023 16:14:48 +0100 Subject: [PATCH 4/6] add review --- review_kurt.md | 101 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 review_kurt.md diff --git a/review_kurt.md b/review_kurt.md new file mode 100644 index 0000000..0b7efc2 --- /dev/null +++ b/review_kurt.md @@ -0,0 +1,101 @@ +## General + +* **Replace `require` with custom errors to save gas:** Consider using custom error handling to replace `require` statements to provide more descriptive error messages without removing any code. + +* **Use `external` instead of `public` functions:** When possible, mark functions as `external` instead of `public` to save gas and enhance security. + + +## GitcoinAttester.sol + +- L8: `import { AttestationRequest, AttestationRequestData, IEAS, Attestation, MultiAttestationRequest, MultiRevocationRequest } from "@ethereum-attestation-service/eas-contracts/contracts/IEAS.sol";` unused imports + - recommendation: remove unused imports +- L20: `IEAS eas` missing visibility + - recommendation: mark explicit as `public` or `private` +- L28, 33, 32, 47, 57, 67, 77, 92: `initialize`, `pause`, `unpause`, `addVerifier`, `removeVerifier`, `setEASAddress`, `submitAttestations`, `revokeAttestations` can be marked as external to save gas. +- `Pausable` is imported, initialized, but never used. + - recommendation: remove Pausable + +## GitcoinPassportDecoder.sol +- L25: `IEAS eas` missing visibility + - recommendation: mark explicit as `public` or `private` +- `Pausable` is imported, initialized, but never used. + - recommendation: remove Pausable + +- L58: `setEASAddress` can be marked as external +- L66: `setGitcoinResolver` can be marked as external +- L74: `setSchemaUID` can be marked as external +- L82: `addProvider` can be marked as external +- L106: `getPassport` can be marked as external +- L95: `getAttestation` should be marked as external and function logic should be moved into internal function to call it in line 116 +- L58: `setEASAddress` missing event +- L66: `setGitcoinResolver` missing event +- L74: `setSchemaUID` missing event +- L82: `addProvider` missing event + +- `getPassport`: The getPassport function is designed to get the attestations of a user and decodes them on chain. Therefore it iterates through two nested for loops. The operations in the for loops can quickly reach the blockchain limitations and hit the gas limits. Since every contract which interacts with this function is at risk, it's highly recommended to revisit the way attestations are stored. + +## GitcoinResolver.sol + +- L8: `import { AttestationRequest, AttestationRequestData, EAS, Attestation, MultiAttestationRequest, IEAS } from "@ethereum-attestation-service/eas-contracts/contracts/EAS.sol";` unused imports + - recommendation: remove unused imports +- L52-65: `gitcoinAttester` can be initialized with zero addess + - recommendation: add zero address check +- L75-77: missing event +- L79-81: missing event +- L83: `pause` can be external +- L87: `unpause` can be external + +- L108-123: `attest` Attestations are made based on the encoded Schema (__right?__) + + If calldata max size = 32kb + and calldata is + + SchemaEncoder( + "uint256[] providers, bytes32[] hashes, uint64[] issuanceDates, uint64[] expirationDates, uint16 providerMapVersion" + ); + + and each type has + uint256 (providers): 32 bytes per element. + bytes32 (hashes): 32 bytes per element. + uint64 (issuanceDates and expirationDates): 8 bytes per element. + uint16 (providerMapVersion): 2 bytes. + // uint256 bitmap hashes issuance expire + => `32,768 bytes = (32 bytes * ceil(x/256)) + x * (32 bytes + 8 bytes + 8 bytes)` + + x represents the number of elements in an array + + which also can be represented by: + `32,768 bytes = (32 bytes * ((x + 255) / 256)) + (x * (32 bytes + 8 bytes + 8 bytes))` + + ``` + 32,768 = (32 * ((x + 255) / 256)) + (x * (32 + 8 + 8)) + 32,768 = (x + 255) / 8 + 48x + 262,144 = x + 255 + 384x + 262,144 = 385x + 255 + 262,144 - 255 = 385x + x = (262,144 - 255) / 385 + x = 261,889 / 385 + x ≈ 679.715 + ``` + + If we consider only the calldata size and no further smart contract operations, the maximum allowed attestation size is ≈ 679. 679 sounds like a pretty high number, but you should also consider that it will be incredible expensive on some chains to just have 50 stamps. Also the `getPassport` in `GitcoinPassportDecoder` will break a way earlier. + + - recommendation: consider storing each stamp in an attestation. + +## GitcoinVeraxPortal.sol + +- L46-57: no zero address check for `_attester`, `_resolver` and `_veraxAttestationRegistry` +- L87: missing event +- L91: misisng event +- L95: missing event +- L102-164: gas optimization: instead of adding `numAttestations += multiAttestationRequests[i].data.length` you could simply pass `numAttestations` as function argument and check at the end of the second for loop if they're equal. Also the length param of the for loops should be cached +- L193: dies the contract `supportsInterface` of `IAbstractPortal` if so many functions are not implemented? + +## GitcoinVerifier.sol + +- L94-114: no zero address check for `_issuer` and `_attester` +- L172+202: cache `*.length` to save gas + +## GitcoinVerifierWithVeraxPortal.sol + +- L30: missing zero address check \ No newline at end of file From 8ccfe6f2f907128d3ec33c5251428764b0c48897 Mon Sep 17 00:00:00 2001 From: Aditya Anand M C Date: Mon, 30 Oct 2023 21:32:38 +0530 Subject: [PATCH 5/6] fourth pass --- review_aditya.md | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/review_aditya.md b/review_aditya.md index 9934be0..44dd850 100644 --- a/review_aditya.md +++ b/review_aditya.md @@ -1,6 +1,6 @@ **General** - Improve natspec documentation to capture input params and return value -- Nitpick but you could explore moving all the interfaces to it's own folder +- Nitpick but you could explore moving all the interfaces to it's # GitcoinVerifier @@ -63,4 +63,10 @@ for (uint i = 0; i < attestationsLength; ) { - `L74`: Emit event `setSchemaUID`. Add zero check - `L82`: Emit event `addProvider`. Add zero check - `L90`: Emit event `createNewVersion`. -- `L146`, `L147`: Store .length as variables as use them in the for loop to save gas \ No newline at end of file +- `L146`, `L147`: Store .length as variables as use them in the for loop to save gas + +**High** + +- `L106`: It looks like we generate 1 attestation for all the stamps. The fact that we have nested loops makes this function quite expensive. Would recommend seeing if this can be done offchain instead. +If the goal of this function is to be consumed by other contracts, I fear it might be expensive for a protocol to make this call, fetch all the stamps and then run their computation. It may exceed the block limit as the project scales +- `L167`: as opposed to hardcoding it to 256 , could we instead have a provider count variable / something else equivalent?i \ No newline at end of file From 7d5f4de3b25ad38c65492a7cfd1650c678b420d6 Mon Sep 17 00:00:00 2001 From: Aditya Anand M C Date: Mon, 30 Oct 2023 21:55:32 +0530 Subject: [PATCH 6/6] final review --- review.md | 148 +++++++++++++++++++++++++++++++++++++++++++++++ review_aditya.md | 72 ----------------------- review_kurt.md | 101 -------------------------------- 3 files changed, 148 insertions(+), 173 deletions(-) create mode 100644 review.md delete mode 100644 review_aditya.md delete mode 100644 review_kurt.md diff --git a/review.md b/review.md new file mode 100644 index 0000000..1645f86 --- /dev/null +++ b/review.md @@ -0,0 +1,148 @@ +## General + +- **Replace `require` with custom errors to save gas:** Consider using custom error handling to replace `require` statements to provide more descriptive error messages without removing any code. +- **Use `external` instead of `public` functions:** When possible, mark functions as `external` instead of `public` to save gas and enhance security. +- Improve `natspec` documentation to capture function `arg` and `return` value. +- Consider moving all the interfaces to it's own folder to make the code organization cleaner. + + +# GitcoinVerifier + +**Questions** +- `L282`: Thoughts on just withdrawing contract balance as opposed to specifying an amount ? +- `L182`: When encoding, why include MULTI_ATTESTATION_REQUEST_TYPEHASH ? Is it cause passport aims to supports another verifier which could use a different format ? + +**Low** +- `L6` : Recommend using `@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol` +- `L43-L80`: Recommend keeping constants with storage variables +- `L94`: Recommend adding natspec for `__GitcoinVerifier_init` +- `L34`: can be made constant with value `GitcoinVerifier`. Remove `L100` during initialize +- `L94-114`: no zero address check for `_issuer` and `_attester` +- `L235`: Consider addressing the TODO via loop. It does increase gas cost but likely worth it unless passport team is confident that the IAM will work as expected +- `L172+L202`: Recommend storing the length in variable as opposed to computing it at every iteration. Gas Optimisation +```javascript +uint256 attestationsLength = attestationRequest.multiAttestationRequest.length; +bytes32[] memory multiAttestHashes = new bytes32[](attestationsLength); +// avoids computing attestationsLength on every iteration +for (uint i = 0; i < attestationsLength; ) { + ... +} +``` + +# GitcoinAttester + +**Questions** +- `L67`: Would it be possible to make `setEASAddress` external ? + +**Low** +- `L8`: `import { AttestationRequest, AttestationRequestData, IEAS, Attestation, MultiAttestationRequest, MultiRevocationRequest } from "@ethereum-attestation-service/eas-contracts/contracts/IEAS.sol";` remove unused imports +- `Pausable` is imported, initialized, but never used. recommendation: remove Pausable +- `L5` : Recommend using `@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol` +- `L20`: Mark the visibility of IEAS as `public` / `private` and then add a external function to read that +- `L28, 33, 32, 47, 57, 67, 77, 92`: `initialize`, `pause`, `unpause`, `addVerifier`, `removeVerifier`, `setEASAddress`, `submitAttestations`, `revokeAttestations` can be marked as external to save gas. +- `L48`, `L59`: Recommend adding custom errors like `VERIFIED_ALREADY_ADDED` / `INVALID_VERIFIED` for gas optimization +- `L68`: Emit event to make it easier to index and have the latest EAS address. Also add zero address check. Why not set this during `initialize` as well +- `L80`/ `L93` The msg.sender == owner() feels like extra code. Would recommend setting owner as verifier during `initialize` and clean update the check to. Also use custom errors instead of `require` +``` if (!verifiers[msg.sender]) revert NOT_VALID_VERIFIER() +``` + + +# GitcoinResolver + +**Questions** + +- Once the flattened attestations are retured from EAS -> GitcoinAttestor -> GitcoinVerifier. How does the Resolver pick it up ? Is it a hook from EAS which invokes `attest` ? + +**Low** +- `L8`: `import { AttestationRequest, AttestationRequestData, EAS, Attestation, MultiAttestationRequest, IEAS } from "@ethereum-attestation-service/eas-contracts/contracts/EAS.sol";` remove unused imports +- `L59` Update check to ensure _gitcoinAttester is not zero +- `L52-65`: `gitcoinAttester` can be initialized with zero addess. Add zero address check +- `L135` Recommend gas optimizing +``` +uint256 attestationsLength = attestations.length +for (uint i = 0; i < attestationsLength; ) { + ... +} +``` +- `L75-77`: missing event +- `L79-81`: missing event +- `L83`: `pause` can be external +- `L87`: `unpause` can be external +- `L108-123``: `attest` Attestations are made based on the encoded Schema (__right?__) + + If calldata max size = 32kb + and calldata is + + SchemaEncoder( + "uint256[] providers, bytes32[] hashes, uint64[] issuanceDates, uint64[] expirationDates, uint16 providerMapVersion" + ); + + and each type has + uint256 (providers): 32 bytes per element. + bytes32 (hashes): 32 bytes per element. + uint64 (issuanceDates and expirationDates): 8 bytes per element. + uint16 (providerMapVersion): 2 bytes. + // uint256 bitmap hashes issuance expire + => `32,768 bytes = (32 bytes * ceil(x/256)) + x * (32 bytes + 8 bytes + 8 bytes)` + + x represents the number of elements in an array + + which also can be represented by: + `32,768 bytes = (32 bytes * ((x + 255) / 256)) + (x * (32 bytes + 8 bytes + 8 bytes))` + + ``` + 32,768 = (32 * ((x + 255) / 256)) + (x * (32 + 8 + 8)) + 32,768 = (x + 255) / 8 + 48x + 262,144 = x + 255 + 384x + 262,144 = 385x + 255 + 262,144 - 255 = 385x + x = (262,144 - 255) / 385 + x = 261,889 / 385 + x ≈ 679.715 + ``` + + If we consider only the calldata size and no further smart contract operations, the maximum allowed attestation size is ≈ 679. 679 sounds like a pretty high number, but you should also consider that it will be incredible expensive on some chains to just have 50 stamps. Also the `getPassport` in `GitcoinPassportDecoder` will break a way earlier. + + - recommendation: consider storing each stamp in an attestation. + + + +# GitcoinPassportDecoder + +**Low** +- `L20`: Mark the visibility of IEAS as `public` / `private` and then add a external function to read that +- `Pausable` is imported, initialized, but never used. Recommendation: remove Pausable +- `L58`: `setEASAddress` can be marked as external +- `L58`: Emit event `setEASAddress`. Add zero check +- `L66`: `setGitcoinResolver` can be marked as external +- `L67`: Emit event `setGitcoinResolver`. Add zero check +- `L74`: `setSchemaUID` can be marked as external +- `L74`: Emit event `setSchemaUID`. Add zero check +- `L82`: `addProvider` can be marked as external +- `L82`: Emit event `addProvider`. Add zero check +- `L95`: `getAttestation` should be marked as external and function logic should be moved into internal function to call it in line 116 +- `L90`: Emit event `createNewVersion`. +- `L106`: `getPassport` can be marked as external +- `L146`, `L147`: Store .length as variables as use them in the for loop to save gas + +**High** + +- `getPassport`: The getPassport function is designed to get the attestations of a user and decodes them on chain. Therefore it iterates through two nested for loops. The operations in the for loops can quickly reach the blockchain limitations and hit the gas limits. Since every contract which interacts with this function is at risk, it's highly recommended to revisit the way attestations are stored. +Consider exploring if there can be an attestation for every stamp and the creating the link between the stamps cab be done off-chain +If the goal of this function is to be consumed by other contracts, I fear it might be expensive for a protocol to make this call, fetch all the stamps and then run their computation. It may exceed the block limit as the project scales +- `L167`: as opposed to hardcoding it to 256 , could we instead have a provider count variable / something else equivalent? + + + +# GitcoinVerifierWithVeraxPortal.sol + +- L30: missing zero address check + +## GitcoinVeraxPortal.sol + +- L46-57: no zero address check for `_attester`, `_resolver` and `_veraxAttestationRegistry` +- L87: missing event +- L91: misisng event +- L95: missing event +- L102-164: gas optimization: instead of adding `numAttestations += multiAttestationRequests[i].data.length` you could simply pass `numAttestations` as function argument and check at the end of the second for loop if they're equal. Also the length param of the for loops should be cached +- L193: dies the contract `supportsInterface` of `IAbstractPortal` if so many functions are not implemented? diff --git a/review_aditya.md b/review_aditya.md deleted file mode 100644 index 44dd850..0000000 --- a/review_aditya.md +++ /dev/null @@ -1,72 +0,0 @@ -**General** -- Improve natspec documentation to capture input params and return value -- Nitpick but you could explore moving all the interfaces to it's - -# GitcoinVerifier - -**Questions** -- `L282`: Thoughts on just withdrawing contract balance as opposed to specifying an amount ? -- `L182`: When encoding, why include MULTI_ATTESTATION_REQUEST_TYPEHASH ? Is it cause passport aims to supports another verifier which could use a different format ? - -**Low** -- `L6` : Recommend using `@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol` -- `L43-L80`: Recommend keeping constants with storage variables -- `L94`: Recommend adding natspec for `__GitcoinVerifier_initno no` -- `L34`: can be made constant with value `GitcoinVerifier`. Remove `L100` during initialize -- `L235`: Consider addressing the TODO via loop. It does increase gas cost but likely worth it unless passport team is confident that the IAM will work as expected -- `L202`: Recommend storing the length in variable as opposed to computing it at every iteration. Gas Optimisation -```javascript -uint256 attestationsLength = attestationRequest.multiAttestationRequest.length; -bytes32[] memory multiAttestHashes = new bytes32[](attestationsLength); -// avoids computing attestationsLength on every iteration -for (uint i = 0; i < attestationsLength; ) { - ... -} -``` - -# GitcoinAttester - -**Questions** -- `L67`: Would it be possible to make `setEASAddress` external ? - -**Low** -- `L5` : Recommend using `@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol` -- `L20`: Mark the visibility of IEAS as `public` / `private` and then add a external function to read that -- `L48`, `L59`: Recommend adding custom errors like `VERIFIED_ALREADY_ADDED` / `INVALID_VERIFIED` for gas optimization -- `L68`: Emit event to make it easier to index and have the latest EAS address. Also add zero address check. Why not set this during `initialize` as well -- `L80`/ `L93` The msg.sender == owner() feels like extra code. Would recommend setting owner as verifier during `initialize` and clean update the check to. Also use custom errors instead of `require` -``` if (!verifiers[msg.sender]) revert NOT_VALID_VERIFIER() -``` - -# GitcoinResolver - -**Questions** - -- Once the flattened attestations are retured from EAS -> GitcoinAttestor -> GitcoinVerifier. How does the Resolver pick it up ? Is it a hook from EAS which invokes `attest` ? - -**Low** -- `L59` Update check to ensure _gitcoinAttester is not zero -- `L135` Recommend gas optimizing -``` -uint256 attestationsLength = attestations.length -for (uint i = 0; i < attestationsLength; ) { - ... -} -``` - -# GitcoinPassportDecoder - -**Low** -- `L20`: Mark the visibility of IEAS as `public` / `private` and then add a external function to read that -- `L58`: Emit event `setEASAddress`. Add zero check -- `L67`: Emit event `setGitcoinResolver`. Add zero check -- `L74`: Emit event `setSchemaUID`. Add zero check -- `L82`: Emit event `addProvider`. Add zero check -- `L90`: Emit event `createNewVersion`. -- `L146`, `L147`: Store .length as variables as use them in the for loop to save gas - -**High** - -- `L106`: It looks like we generate 1 attestation for all the stamps. The fact that we have nested loops makes this function quite expensive. Would recommend seeing if this can be done offchain instead. -If the goal of this function is to be consumed by other contracts, I fear it might be expensive for a protocol to make this call, fetch all the stamps and then run their computation. It may exceed the block limit as the project scales -- `L167`: as opposed to hardcoding it to 256 , could we instead have a provider count variable / something else equivalent?i \ No newline at end of file diff --git a/review_kurt.md b/review_kurt.md deleted file mode 100644 index 0b7efc2..0000000 --- a/review_kurt.md +++ /dev/null @@ -1,101 +0,0 @@ -## General - -* **Replace `require` with custom errors to save gas:** Consider using custom error handling to replace `require` statements to provide more descriptive error messages without removing any code. - -* **Use `external` instead of `public` functions:** When possible, mark functions as `external` instead of `public` to save gas and enhance security. - - -## GitcoinAttester.sol - -- L8: `import { AttestationRequest, AttestationRequestData, IEAS, Attestation, MultiAttestationRequest, MultiRevocationRequest } from "@ethereum-attestation-service/eas-contracts/contracts/IEAS.sol";` unused imports - - recommendation: remove unused imports -- L20: `IEAS eas` missing visibility - - recommendation: mark explicit as `public` or `private` -- L28, 33, 32, 47, 57, 67, 77, 92: `initialize`, `pause`, `unpause`, `addVerifier`, `removeVerifier`, `setEASAddress`, `submitAttestations`, `revokeAttestations` can be marked as external to save gas. -- `Pausable` is imported, initialized, but never used. - - recommendation: remove Pausable - -## GitcoinPassportDecoder.sol -- L25: `IEAS eas` missing visibility - - recommendation: mark explicit as `public` or `private` -- `Pausable` is imported, initialized, but never used. - - recommendation: remove Pausable - -- L58: `setEASAddress` can be marked as external -- L66: `setGitcoinResolver` can be marked as external -- L74: `setSchemaUID` can be marked as external -- L82: `addProvider` can be marked as external -- L106: `getPassport` can be marked as external -- L95: `getAttestation` should be marked as external and function logic should be moved into internal function to call it in line 116 -- L58: `setEASAddress` missing event -- L66: `setGitcoinResolver` missing event -- L74: `setSchemaUID` missing event -- L82: `addProvider` missing event - -- `getPassport`: The getPassport function is designed to get the attestations of a user and decodes them on chain. Therefore it iterates through two nested for loops. The operations in the for loops can quickly reach the blockchain limitations and hit the gas limits. Since every contract which interacts with this function is at risk, it's highly recommended to revisit the way attestations are stored. - -## GitcoinResolver.sol - -- L8: `import { AttestationRequest, AttestationRequestData, EAS, Attestation, MultiAttestationRequest, IEAS } from "@ethereum-attestation-service/eas-contracts/contracts/EAS.sol";` unused imports - - recommendation: remove unused imports -- L52-65: `gitcoinAttester` can be initialized with zero addess - - recommendation: add zero address check -- L75-77: missing event -- L79-81: missing event -- L83: `pause` can be external -- L87: `unpause` can be external - -- L108-123: `attest` Attestations are made based on the encoded Schema (__right?__) - - If calldata max size = 32kb - and calldata is - - SchemaEncoder( - "uint256[] providers, bytes32[] hashes, uint64[] issuanceDates, uint64[] expirationDates, uint16 providerMapVersion" - ); - - and each type has - uint256 (providers): 32 bytes per element. - bytes32 (hashes): 32 bytes per element. - uint64 (issuanceDates and expirationDates): 8 bytes per element. - uint16 (providerMapVersion): 2 bytes. - // uint256 bitmap hashes issuance expire - => `32,768 bytes = (32 bytes * ceil(x/256)) + x * (32 bytes + 8 bytes + 8 bytes)` - - x represents the number of elements in an array - - which also can be represented by: - `32,768 bytes = (32 bytes * ((x + 255) / 256)) + (x * (32 bytes + 8 bytes + 8 bytes))` - - ``` - 32,768 = (32 * ((x + 255) / 256)) + (x * (32 + 8 + 8)) - 32,768 = (x + 255) / 8 + 48x - 262,144 = x + 255 + 384x - 262,144 = 385x + 255 - 262,144 - 255 = 385x - x = (262,144 - 255) / 385 - x = 261,889 / 385 - x ≈ 679.715 - ``` - - If we consider only the calldata size and no further smart contract operations, the maximum allowed attestation size is ≈ 679. 679 sounds like a pretty high number, but you should also consider that it will be incredible expensive on some chains to just have 50 stamps. Also the `getPassport` in `GitcoinPassportDecoder` will break a way earlier. - - - recommendation: consider storing each stamp in an attestation. - -## GitcoinVeraxPortal.sol - -- L46-57: no zero address check for `_attester`, `_resolver` and `_veraxAttestationRegistry` -- L87: missing event -- L91: misisng event -- L95: missing event -- L102-164: gas optimization: instead of adding `numAttestations += multiAttestationRequests[i].data.length` you could simply pass `numAttestations` as function argument and check at the end of the second for loop if they're equal. Also the length param of the for loops should be cached -- L193: dies the contract `supportsInterface` of `IAbstractPortal` if so many functions are not implemented? - -## GitcoinVerifier.sol - -- L94-114: no zero address check for `_issuer` and `_attester` -- L172+202: cache `*.length` to save gas - -## GitcoinVerifierWithVeraxPortal.sol - -- L30: missing zero address check \ No newline at end of file