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

Aboudjem · 2024-07-31T00:37:31Z

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

Issue: Lack of authorization control in the fallback function.
Affected Functions: fallback function in ModuleManager.
Fix: Implement proper authorization control to ensure only authorized entities can invoke it.

…n.tree

github-actions · 2024-07-31T00:39:06Z

🤖 Slither Analysis Report 🔎

Slither report

# Slither report

THIS CHECKLIST IS NOT COMPLETE. Use --show-ignored-findings to show all the results.
Summary

constable-states (2 results) (Optimization)

constable-states

Impact: Optimization
🔴 Confidence: High

ID-0
RegistryAdapter.registry should be constant

base/RegistryAdapter.sol#L12

ID-1
RegistryFactory.threshold should be constant

factory/RegistryFactory.sol#L39

_This comment was automatically generated by the GitHub Actions workflow._

github-actions · 2024-07-31T00:40:55Z

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 · 2024-07-31T00:48:50Z

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 ⚠️
contracts/Nexus.sol	76.47%	4 Missing ⚠️
contracts/base/BaseAccount.sol	0.00%	2 Missing ⚠️

Additional details and impacted files

@@                 Coverage Diff                 @@
##             fix/security-m01     #120   +/-   ##
===================================================
  Coverage                    ?   71.73%           
===================================================
  Files                       ?       13           
  Lines                       ?      697           
  Branches                    ?      133           
===================================================
  Hits                        ?      500           
  Misses                      ?      197           
  Partials                    ?        0

Files	Coverage Δ
contracts/base/Storage.sol	`66.66% <ø> (ø)`
contracts/modules/validators/K1Validator.sol	`83.33% <100.00%> (ø)`
contracts/base/BaseAccount.sol	`62.85% <0.00%> (ø)`
contracts/Nexus.sol	`62.64% <76.47%> (ø)`
contracts/base/ModuleManager.sol	`85.97% <75.00%> (ø)`

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a75c21...3f48ffb. Read the comment docs.

livingrockrises · 2024-08-10T14:34:42Z

contracts/Nexus.sol

@@ -137,15 +138,22 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra
 function executeFromExecutor(
 ExecutionMode mode,
 bytes calldata executionCalldata
- ) external payable onlyExecutorModule withHook withRegistry(msg.sender, MODULE_TYPE_EXECUTOR) returns (bytes[] memory returnData) {
+ )


can we not touch linting as far as this PR goes?

livingrockrises · 2024-08-10T14:35:20Z

contracts/base/BaseAccount.sol

@@ -23,14 +25,10 @@ import { IBaseAccount } from "../interfaces/base/IBaseAccount.sol";
 /// @author @filmakarov | Biconomy | filipp.makarov@biconomy.io
 /// @author @zeroknots | Rhinestone.wtf | zeroknots.eth
 /// Special thanks to the Solady team for foundational contributions: https://github.com/Vectorized/solady
-contract BaseAccount is IBaseAccount {
+contract BaseAccount is Storage, IBaseAccount {


I moved the _ENTRYPOINT to storage, as it needs to be used by both BaseAccount and ModuleManager for the new onlyAuthorized

why would you do that?
it's not a storage
it's immutable : address internal immutable _ENTRYPOINT;

what's new onlyAuthorized for?

onlyAuthorized is checking msg.sender is EP, self or Executor while other modifier are exclusively checking EP EP+self or Executor only

if the naming is a problem we can change it

livingrockrises · 2024-08-10T14:37:53Z

@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 · 2024-08-12T10:28:20Z

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

livingrockrises · 2024-08-12T12:34:55Z

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 · 2024-08-12T14:51:02Z

@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 · 2024-08-12T15:00:12Z

@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 · 2024-08-12T15:01:20Z

@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

Aboudjem added 7 commits July 31, 2024 01:25

feat: Add validation for 's' value in K1Validator

60b93a9

feat: Add Storage contract and update BaseAccount to use it

ef5c7b6

add new modifier onlyAuthorized

303369f

feat: fix account creation test in TestK1ValidatorFactory_Deployments

6f94787

feat: Add _ENTRYPOINT address to Storage contract

5a9eacf

refactor: Update fallback function tests in TextNexus_FallbackFunctio…

6ac9161

…n.tree

feat: Improve validation for 's' value in K1Validator

3f48ffb

Aboudjem requested review from filmakarov, ankurdubey521, VGabriel45 and livingrockrises July 31, 2024 00:37

Aboudjem self-assigned this Jul 31, 2024

Aboudjem changed the title ~~Fix/security m03~~ 🔒 M-03 - Add Authorization Control to Fallback Function Jul 31, 2024

livingrockrises reviewed Aug 10, 2024

View reviewed changes

Aboudjem closed this Aug 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Aboudjem commented Jul 31, 2024

github-actions bot commented Jul 31, 2024

constable-states

github-actions bot commented Jul 31, 2024

codecov bot commented Jul 31, 2024

livingrockrises Aug 10, 2024

livingrockrises Aug 10, 2024

Aboudjem Aug 12, 2024

livingrockrises Aug 12, 2024

Aboudjem Aug 12, 2024

livingrockrises commented Aug 10, 2024

VGabriel45 commented Aug 12, 2024

livingrockrises commented Aug 12, 2024

Aboudjem commented Aug 12, 2024 •

edited

Loading

livingrockrises commented Aug 12, 2024

Aboudjem commented Aug 12, 2024

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

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

Conversation

Aboudjem commented Jul 31, 2024

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

github-actions bot commented Jul 31, 2024

🤖 Slither Analysis Report 🔎

Slither report

constable-states

github-actions bot commented Jul 31, 2024

Changes to gas cost

🧾 Summary (5% most significant diffs)

codecov bot commented Jul 31, 2024

Codecov Report

livingrockrises Aug 10, 2024

Choose a reason for hiding this comment

livingrockrises Aug 10, 2024

Choose a reason for hiding this comment

Aboudjem Aug 12, 2024

Choose a reason for hiding this comment

livingrockrises Aug 12, 2024

Choose a reason for hiding this comment

Aboudjem Aug 12, 2024

Choose a reason for hiding this comment

livingrockrises commented Aug 10, 2024

VGabriel45 commented Aug 12, 2024

livingrockrises commented Aug 12, 2024

Aboudjem commented Aug 12, 2024 • edited Loading

livingrockrises commented Aug 12, 2024

Aboudjem commented Aug 12, 2024

Aboudjem commented Aug 12, 2024 •

edited

Loading