search

bcnmy / nexus

Nexus by Biconomy: ERC-7579 Modular Smart Account for Enhanced Account Abstraction
https://github.com/bcnmy/nexus/wiki
MIT License
27 stars 5 forks source link

🔒 M-03 - Add Authorization Control to Fallback Function #120

Closed Aboudjem closed 3 months ago

Aboudjem commented 3 months ago

M-03. Anyone can call the fallbackFunction because of missing authorization control

github-actions[bot] commented 3 months ago

:robot: Slither Analysis Report :mag_right:

Slither report

# Slither report **THIS CHECKLIST IS NOT COMPLETE**. Use `--show-ignored-findings` to show all the results. Summary - [constable-states](#constable-states) (2 results) (Optimization) ## constable-states Impact: Optimization :red_circle: Confidence: High - [ ] ID-0 [RegistryAdapter.registry]([base/RegistryAdapter.sol#L12](https://github.com/bcnmy/nexus/blob/07c012a965078a3e22922aaf032d37609307c694/contracts/contracts/base/RegistryAdapter.sol#L12)) should be constant [base/RegistryAdapter.sol#L12](https://github.com/bcnmy/nexus/blob/07c012a965078a3e22922aaf032d37609307c694/contracts/contracts/base/RegistryAdapter.sol#L12) - [ ] ID-1 [RegistryFactory.threshold]([factory/RegistryFactory.sol#L39](https://github.com/bcnmy/nexus/blob/07c012a965078a3e22922aaf032d37609307c694/contracts/contracts/factory/RegistryFactory.sol#L39)) should be constant [factory/RegistryFactory.sol#L39](https://github.com/bcnmy/nexus/blob/07c012a965078a3e22922aaf032d37609307c694/contracts/contracts/factory/RegistryFactory.sol#L39)

This comment was automatically generated by the GitHub Actions workflow.

github-actions[bot] commented 3 months ago

Changes to gas cost

Generated at commit: 07c012a965078a3e22922aaf032d37609307c694, compared to commit: 5a75c21571a1cca8431b5c7b1cd1737e8f0ca367

🧾 Summary (5% most significant diffs)

Contract Method Avg (+/-) %
Full diff report 👇
| Contract | Deployment Cost (+/-) | Method | Min (+/-) | % | Avg (+/-) | % | Median (+/-) | % | Max (+/-) | % | # Calls (+/-) | |:-|-:|:-|-:|-:|-:|-:|-:|-:|-:|-:|-:| | **Nexus** | 5,432,856 (+30,964) | | | | | | | | | | | | **Bootstrap** | 2,460,986 (+31,040) | | | | | | | | | | |
codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 74.41860% with 11 lines in your changes missing coverage. Please review.

Please upload report for BASE (fix/security-m01@5a75c21). Learn more about missing BASE report.

Files Patch % Lines
contracts/base/ModuleManager.sol 75.00% 5 Missing :warning:
contracts/Nexus.sol 76.47% 4 Missing :warning:
contracts/base/BaseAccount.sol 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## fix/security-m01 #120 +/- ## =================================================== Coverage ? 71.73% =================================================== Files ? 13 Lines ? 697 Branches ? 133 =================================================== Hits ? 500 Misses ? 197 Partials ? 0 ``` | [Files](https://app.codecov.io/gh/bcnmy/nexus/pull/120?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy) | Coverage Δ | | |---|---|---| | [contracts/base/Storage.sol](https://app.codecov.io/gh/bcnmy/nexus/pull/120?src=pr&el=tree&filepath=contracts%2Fbase%2FStorage.sol&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy#diff-Y29udHJhY3RzL2Jhc2UvU3RvcmFnZS5zb2w=) | `66.66% <ø> (ø)` | | | [contracts/modules/validators/K1Validator.sol](https://app.codecov.io/gh/bcnmy/nexus/pull/120?src=pr&el=tree&filepath=contracts%2Fmodules%2Fvalidators%2FK1Validator.sol&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy#diff-Y29udHJhY3RzL21vZHVsZXMvdmFsaWRhdG9ycy9LMVZhbGlkYXRvci5zb2w=) | `83.33% <100.00%> (ø)` | | | [contracts/base/BaseAccount.sol](https://app.codecov.io/gh/bcnmy/nexus/pull/120?src=pr&el=tree&filepath=contracts%2Fbase%2FBaseAccount.sol&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy#diff-Y29udHJhY3RzL2Jhc2UvQmFzZUFjY291bnQuc29s) | `62.85% <0.00%> (ø)` | | | [contracts/Nexus.sol](https://app.codecov.io/gh/bcnmy/nexus/pull/120?src=pr&el=tree&filepath=contracts%2FNexus.sol&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy#diff-Y29udHJhY3RzL05leHVzLnNvbA==) | `62.64% <76.47%> (ø)` | | | [contracts/base/ModuleManager.sol](https://app.codecov.io/gh/bcnmy/nexus/pull/120?src=pr&el=tree&filepath=contracts%2Fbase%2FModuleManager.sol&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy#diff-Y29udHJhY3RzL2Jhc2UvTW9kdWxlTWFuYWdlci5zb2w=) | `85.97% <75.00%> (ø)` | | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/bcnmy/nexus/pull/120?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/bcnmy/nexus/pull/120?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy). Last update [5a75c21...3f48ffb](https://app.codecov.io/gh/bcnmy/nexus/pull/120?dropdown=coverage&src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy).
livingrockrises commented 3 months ago

@Aboudjem

what is done specific to this?

Fix: Implement proper authorization control to ensure only authorized entities can invoke it

I don't see changes related to it. we also discussed fallback handler should implement it's own auth control and we are gonna make changes on the EIP PR. cc @filmakarov

can you close this PR and redo it if only there are additional test cases. bunch of linting makes it very hard to review..

VGabriel45 commented 3 months ago

Should this be a remediation for this https://codehawks.cyfrin.io/c/2024-07-biconomy/s/179 ?

livingrockrises commented 3 months ago

Should this be a remediation for this https://codehawks.cyfrin.io/c/2024-07-biconomy/s/179 ?

No, anyone should be able to invoke fallback handlers. the actual access control (if required) should be implemented in the handlers themselves..

Aboudjem commented 3 months ago

@Aboudjem

what is done specific to this?

Fix: Implement proper authorization control to ensure only authorized entities can invoke it

I don't see changes related to it. we also discussed fallback handler should implement it's own auth control and we are gonna make changes on the EIP PR. cc @filmakarov

can you close this PR and redo it if only there are additional test cases. bunch of linting makes it very hard to review..

Implement proper authorization control in the fallback function to ensure that only authorized entities can invoke it. This can be achieved by adding a modifier to check the sender's authorization before routing the call to the fallback handler. The existing onlyEntryPointOrSelf modifier could be used or an new modifier also including executorModuls might be appropriate.

At this point, the entrypoint and the onlyEntrypoint or onlySelfOrEntrypoint are not visible, any suggestion?

Yes my husky script messed a bit with the linter

livingrockrises commented 3 months ago

@Aboudjem what is done specific to this?

Fix: Implement proper authorization control to ensure only authorized entities can invoke it

I don't see changes related to it. we also discussed fallback handler should implement it's own auth control and we are gonna make changes on the EIP PR. cc @filmakarov can you close this PR and redo it if only there are additional test cases. bunch of linting makes it very hard to review..

Implement proper authorization control in the fallback function to ensure that only authorized entities can invoke it. This can be achieved by adding a modifier to check the sender's authorization before routing the call to the fallback handler. The existing onlyEntryPointOrSelf modifier could be used or an new modifier also including executorModuls might be appropriate.

At this point, the entrypoint and the onlyEntrypoint or onlySelfOrEntrypoint are not visible, any suggestion?

Yes my husky script messed a bit with the linter

dont think any of this change makes sense.

Aboudjem commented 3 months ago

@Aboudjem what is done specific to this?

Fix: Implement proper authorization control to ensure only authorized entities can invoke it

I don't see changes related to it. we also discussed fallback handler should implement it's own auth control and we are gonna make changes on the EIP PR. cc @filmakarov can you close this PR and redo it if only there are additional test cases. bunch of linting makes it very hard to review..

Implement proper authorization control in the fallback function to ensure that only authorized entities can invoke it. This can be achieved by adding a modifier to check the sender's authorization before routing the call to the fallback handler. The existing onlyEntryPointOrSelf modifier could be used or an new modifier also including executorModuls might be appropriate. At this point, the entrypoint and the onlyEntrypoint or onlySelfOrEntrypoint are not visible, any suggestion? Yes my husky script messed a bit with the linter

dont think any of this change makes sense.

then close the PR