Skip to content

Conversation

stevieraykatz
Copy link
Collaborator

@stevieraykatz stevieraykatz commented Oct 31, 2024

In this version of the ERC1155 discount validator, we accept an array of uints upon initialization, setting the approved token Ids for the token collection.

Upon calling the isValidDiscountRegistration, an integrator should pack the token Ids in an array and pass this as the validationData bytes. For example, to check for token ids 0, 1 and 2, an integrator would set validationData using abi.encode([0,1,2]).

The token Ids are decoded into an uint[] ids object and each is checked for

  1. validator approval, and
  2. a nonzero balance for claimer

The validator returns true if both criteria are true for a single tokenId, else false.

@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Oct 31, 2024

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@stevieraykatz stevieraykatz marked this pull request as ready for review November 4, 2024 18:26
@abdulla-cb abdulla-cb self-requested a review November 4, 2024 23:13
@cb-heimdall
Copy link
Collaborator

Review Error for abdulla-cb @ 2024-11-04 23:14:04 UTC
User cannot review their own commit

Copy link
Collaborator

@cb-elileers cb-elileers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good other than the potential reentrancy in the balance call. Made a suggestion on a potential fix.

uint256[] memory ids = abi.decode(validationData, (uint256[]));
for (uint256 i; i < ids.length; i++) {
uint256 id = ids[i];
if (approvedTokenIds[id] && token.balanceOf(claimer, id) > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This technically adds a reentrancy risk, which should be mitigated by the fact that this a token that we are specifying.

Can we wrap this external call in a staticcall to remove any risk? It will also make it easier when we want to add more validators, nobody will have to review the balance function beforehand.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, updated to include a staticcall helper method for balance checks.

@cb-heimdall
Copy link
Collaborator

Review Error for cb-elileers @ 2024-11-04 23:40:11 UTC
User failed mfa authentication, public email is not set on your github profile. see go/mfa-help

Copy link
Collaborator

@cb-elileers cb-elileers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the change! Looks good to me :)

}

/// @notice Helper for staticcalling getBalance to avoid reentrancy vector.
function _getBalance(address claimer, uint256 id) internal view returns (uint256) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. I like that you pulled in the OZ Address library for this instead of writing a new implementation.

@stevieraykatz stevieraykatz merged commit 8f163b9 into main Nov 5, 2024
6 checks passed
@stevieraykatz stevieraykatz deleted the new-erc1155-discount-validator branch November 5, 2024 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants