Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: (C4 #115) add input validations when setting data in LSP6SetDataModule #679

Merged
merged 3 commits into from
Aug 23, 2023
Merged

refactor: (C4 #115) add input validations when setting data in LSP6SetDataModule #679

merged 3 commits into from
Aug 23, 2023

Conversation

b00ste
Copy link
Member

@b00ste b00ste commented Aug 14, 2023

What does this PR introduce?

PR Checklist

♻️ Refactor

Add input validations for the data values of specific data keys.

  • Wrote Tests
  • Wrote & Generated Documentation (readme/natspec/dodoc)
  • Ran npm run lint && npm run lint:solidity (solhint)
  • Ran npm run format (prettier)
  • Ran npm run build
  • Ran npm run test

@Hugoo
Copy link
Contributor

Hugoo commented Aug 14, 2023

Task linked: DEV-7685 [C4#115] Missing input validations

@github-actions
Copy link
Contributor

github-actions bot commented Aug 14, 2023 •
edited
Loading

Changes to gas cost

Generated at commit: 191c6dc91eadaab475de18c31ee8054f242006c5, compared to commit: 9db488aad9065f88cfc3c5f43f47b1fab35fc5f0

🧾 Summary (10% most significant diffs)

Contract Method Avg (+/-) %
LSP6SetDataRestrictedController supportsInterface +55 ❌ +10.24%
LSP6SetDataUnrestrictedController supportsInterface +55 ❌ +10.24%
LSP6ExecuteRestrictedController supportsInterface +55 ❌ +9.82%
LSP6ExecuteUnrestrictedController supportsInterface +55 ❌ +9.82%
Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
LSP6SetDataRestrictedController 2,860,468 (+21,623) execute
givePermissionsToController
restrictControllerToERC725YKeys
supportsInterface		 18,490 (+136)
120,634 (+107)
138,884 (+55)
592 (+55)		 +0.74%
+0.09%
+0.04%
+10.24%		 28,535 (+82)
120,634 (+107)
138,884 (+55)
592 (+55)		 +0.29%
+0.09%
+0.04%
+10.24%		 28,535 (+82)
120,634 (+107)
138,884 (+55)
592 (+55)		 +0.29%
+0.09%
+0.04%
+10.24%		 38,581 (+28)
120,634 (+107)
138,884 (+55)
592 (+55)		 +0.07%
+0.09%
+0.04%
+10.24%		 2 (0)
1 (0)
1 (0)
4 (0)
LSP6SetDataUnrestrictedController 2,860,468 (+21,623) execute
givePermissionsToController
restrictControllerToERC725YKeys
supportsInterface		 18,490 (+136)
126,634 (+107)
147,384 (+55)
592 (+55)		 +0.74%
+0.08%
+0.04%
+10.24%		 28,535 (+82)
126,634 (+107)
147,384 (+55)
592 (+55)		 +0.29%
+0.08%
+0.04%
+10.24%		 28,535 (+82)
126,634 (+107)
147,384 (+55)
592 (+55)		 +0.29%
+0.08%
+0.04%
+10.24%		 38,581 (+28)
126,634 (+107)
147,384 (+55)
592 (+55)		 +0.07%
+0.08%
+0.04%
+10.24%		 2 (0)
1 (0)
1 (0)
4 (0)
LSP6ExecuteRestrictedController 2,875,691 (+21,631) lsp20VerifyCall
supportsInterface
transferLYXToEOA
transferLYXToUP
transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomEOA
transferTokensToRandomUP		 14,896 (-3)
615 (+55)
56,165 (-998)
31,496 (+31)
130,008 (-856)
237,855 (-857)
76,025 (-1,066)
210,610 (-1,069)		 -0.02%
+9.82%
-1.75%
+0.10%
-0.65%
-0.36%
-1.38%
-0.51%		 17,032 (-3)
615 (+55)
56,165 (-998)
31,496 (+31)
130,008 (-856)
237,855 (-857)
76,025 (-1,066)
210,610 (-1,069)		 -0.02%
+9.82%
-1.75%
+0.10%
-0.65%
-0.36%
-1.38%
-0.51%		 17,745 (-3)
615 (+55)
56,165 (-998)
31,496 (+31)
130,008 (-856)
237,855 (-857)
76,025 (-1,066)
210,610 (-1,069)		 -0.02%
+9.82%
-1.75%
+0.10%
-0.65%
-0.36%
-1.38%
-0.51%		 17,745 (-3)
615 (+55)
56,165 (-998)
31,496 (+31)
130,008 (-856)
237,855 (-857)
76,025 (-1,066)
210,610 (-1,069)		 -0.02%
+9.82%
-1.75%
+0.10%
-0.65%
-0.36%
-1.38%
-0.51%		 8 (0)
24 (0)
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
LSP6ExecuteUnrestrictedController 2,875,691 (+21,631) lsp20VerifyCall
supportsInterface
transferLYXToEOA
transferLYXToUP
transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomEOA
transferTokensToRandomUP		 14,896 (-3)
615 (+55)
56,795 (+31)
33,496 (+31)
128,757 (+57)
236,604 (+55)
74,461 (+74)
209,046 (+71)		 -0.02%
+9.82%
+0.05%
+0.09%
+0.04%
+0.02%
+0.10%
+0.03%		 17,032 (-3)
615 (+55)
56,795 (+31)
33,496 (+31)
128,757 (+57)
236,604 (+55)
74,461 (+74)
209,046 (+71)		 -0.02%
+9.82%
+0.05%
+0.09%
+0.04%
+0.02%
+0.10%
+0.03%		 17,745 (-3)
615 (+55)
56,795 (+31)
33,496 (+31)
128,757 (+57)
236,604 (+55)
74,461 (+74)
209,046 (+71)		 -0.02%
+9.82%
+0.05%
+0.09%
+0.04%
+0.02%
+0.10%
+0.03%		 17,745 (-3)
615 (+55)
56,795 (+31)
33,496 (+31)
128,757 (+57)
236,604 (+55)
74,461 (+74)
209,046 (+71)		 -0.02%
+9.82%
+0.05%
+0.09%
+0.04%
+0.02%
+0.10%
+0.03%		 8 (0)
24 (0)
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)

@b00ste b00ste force-pushed the DEV-7685 branch 7 times, most recently from 8aeaf49 to 82c1d96 Compare August 15, 2023 13:28
CJ42
CJ42 requested changes Aug 15, 2023
View reviewed changes
contracts/LSP6KeyManager/LSP6Errors.sol Outdated Show resolved Hide resolved
contracts/LSP6KeyManager/LSP6Errors.sol Outdated Show resolved Hide resolved
contracts/LSP6KeyManager/LSP6Errors.sol Outdated Show resolved Hide resolved
contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol Outdated Show resolved Hide resolved
contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol Outdated Show resolved Hide resolved
docs/contracts/LSP6KeyManager/LSP6KeyManager.md Outdated Show resolved Hide resolved
tests/LSP20CallVerification/LSP6/Admin/PermissionChangeAddExtensions.test.ts Show resolved Hide resolved
CJ42
CJ42 requested changes Aug 15, 2023
View reviewed changes
Copy link
Member

@CJ42 CJ42 left a comment

Choose a reason for hiding this comment

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

Let's please review the newly added errors. This is adding 66k gas on deployment. It's way too much for just one check and two new custom errors.

Let's see if we can reduce this gas by some small refactoring or re-using the other errors 🙏

@b00ste b00ste force-pushed the DEV-7685 branch 4 times, most recently from 84ee0bf to 65cab65 Compare August 22, 2023 13:58
YamenMerhi
YamenMerhi reviewed Aug 22, 2023
View reviewed changes
Copy link
Member

@YamenMerhi YamenMerhi left a comment

Choose a reason for hiding this comment

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

Was it discussed to have it strictly the length or the minimum length?

contracts/LSP6KeyManager/LSP6Errors.sol Outdated Show resolved Hide resolved
@CJ42
Copy link
Member

CJ42 commented Aug 22, 2023

Was it discussed to have it strictly the length or the minimum length?

Good point. I remember @frozeman saying it should be at least the minimum length. Any extra byte we ignore it.

skimaharvey
skimaharvey approved these changes Aug 22, 2023
View reviewed changes
Copy link
Member

@skimaharvey skimaharvey left a comment

Choose a reason for hiding this comment

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

LGTM

@skimaharvey
Copy link
Member

Was it discussed to have it strictly the length or the minimum length?

Good point. I remember @frozeman saying it should be at least the minimum length. Any extra byte we ignore it.
I believe for most strictly makes more sense as we are casting after but could be wrong

CJ42
CJ42 reviewed Aug 23, 2023
View reviewed changes
Copy link
Member

@CJ42 CJ42 left a comment

Choose a reason for hiding this comment

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

Added some final suggestions for the Natspec

contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol Outdated Show resolved Hide resolved
contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol Outdated Show resolved Hide resolved
contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol Outdated Show resolved Hide resolved
contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol Show resolved Hide resolved
contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol Outdated Show resolved Hide resolved
@b00ste b00ste force-pushed the DEV-7685 branch 5 times, most recently from e16f060 to 67e16e1 Compare August 23, 2023 10:45
@CJ42 CJ42 changed the title refactor: add input validations refactor: add input validations in LSP1UniversalReceiverDelegateUP Aug 23, 2023
@CJ42 CJ42 changed the title refactor: add input validations in LSP1UniversalReceiverDelegateUP refactor: add input validations when setting data in LSP6SetDataModule Aug 23, 2023
@CJ42 CJ42 merged commit 93c5a2a into develop Aug 23, 2023
26 checks passed
@CJ42 CJ42 deleted the DEV-7685 branch August 23, 2023 14:24
@CJ42 CJ42 changed the title refactor: add input validations when setting data in LSP6SetDataModule refactor: (C4 #115) add input validations when setting data in LSP6SetDataModule Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YamenMerhi YamenMerhi YamenMerhi left review comments

@CJ42 CJ42 CJ42 approved these changes

@skimaharvey skimaharvey skimaharvey approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@b00ste @Hugoo @CJ42 @skimaharvey @YamenMerhi