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.

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

Added Test Cases per Zellic audit questions #138

Merged

Conversation

filmakarov
Copy link
Collaborator

@filmakarov filmakarov commented Sep 21, 2023

Summary

  • In the BatchedSessionRouterModule, added a check to ensure that the decoded sessionData array contains enough elements for all the actions and corresponding tests
  • For the ERC20SessionValidationModule, added tests to ensure it reverts when userOp.callData is too short or when the length for the inner bytes array is not correct

Related Issue: SMA-143, and SMA-144

Change Type

  • Unit Tests
  • Bug Fix

Checklist

  • My code follows this project's style guidelines
  • I've reviewed my own code
  • I've added comments for any hard-to-understand areas
  • I've updated the documentation if necessary
  • My changes generate no new warnings
  • I've added tests that prove my fix is effective or my feature works
  • All unit tests pass locally with my changes
  • Any dependent changes have been merged and published

P.S. The branch name is fix/ not /fixes because it was created before the discussion about naming convention. Next time will make it fixes/.

@linear
Copy link

linear bot commented Sep 21, 2023

SMA-143 Add length related checks to ERC20 SVM

> - In the ERC20SessionValidationModule, within the validateSessionUserOp function, should there be a check for the minimum length of _op.callData and a maximum length not exceeding 4 + offset + 32 + length?

I suppose, that the minimum length check is being done implicitly. if it is less then 4 bytes, the selector check will revert

if it is less than 68 bytes, decoding tokenAddr and callValue will fail. 

But you're right, that we maybe should add a test case to check that it definitely fails.

For the maximum length check, do you mean the case when the callData is some arbitrary data, not the correctly encoded one, and the length can be taken from some random bytes?

I think yes, maybe we need to test the behavior of ERC20 SVM in this case and see if the explicit check in the smart contract is needed or it still reverts in case of incorrectly encoded callData.

ankurdubey521
ankurdubey521 previously approved these changes Sep 21, 2023
Copy link
Collaborator

@ankurdubey521 ankurdubey521 left a comment

Choose a reason for hiding this comment

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

LGTM, but I think you need to fix a few linting errors

Aboudjem
Aboudjem previously approved these changes Sep 21, 2023
@filmakarov filmakarov dismissed stale reviews from Aboudjem and ankurdubey521 via 38e283a September 21, 2023 09:01
@filmakarov
Copy link
Collaborator Author

@ankurdubey521 @Aboudjem Sorry guys, it requires approvals from you again.
Next time will fix the linting first before requesting the reviews

@Aboudjem
Copy link
Contributor

@ankurdubey521 @Aboudjem Sorry guys, it requires approvals from you again. Next time will fix the linting first before requesting the reviews

No worries ! the husky hook didn't work?

@Aboudjem Aboudjem self-requested a review September 21, 2023 09:52
@filmakarov
Copy link
Collaborator Author

No worries ! the husky hook didn't work?

I skipped it initially :)

@ankurdubey521 ankurdubey521 self-requested a review September 21, 2023 11:16
@filmakarov filmakarov merged commit 3bf128e into master Sep 21, 2023
2 checks passed
@filmakarov filmakarov deleted the fix/SMA-143-144-length-test-cases-per-Zellic-questions branch September 21, 2023 12:03
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.

3 participants