AUT_EXT_VotingRoles_v1.sol/PP_Streaming_v1.sol - edge-case scenario of voting would allow cancelling running payments

[hats-bug-reporter[bot]]

Github username: @PlamenTSV Twitter username: @p_tsanev Submission hash (on-chain): 0x9778ef5c9a8ff46dfc942aa22d00101fe3d9d3fa68198e10713ca05f363141ae Severity: medium

Description: Description\ The AUT_EXT_VotingRoles_v1.sol implements a voting system which allows for adding, removing voters and executing arbitrary operations when the action's votes passes a given threshold. Said threshold is uncapped at the bottom, meaning that is such a edge-case any malicious voter would be able to execute arbitrary transactions from the AUT_EXT_VotingRoles_v1.sol and bypass any onlyModule modifier inside other contracts.

Attack Scenario\ This is a very specific edge-case requiring the threshold to be either 1 or 0, which are both possible valid states. This means that any proposed action will immediately pass and we enter:

(result, returnData) = motion_.target.call(motion_.action);

This external call would be made from the voter contract, not from the user who executes the motion. Since the voter contract is a module inside of the Orchestrator system, we can execute any onlyModule function. The most impacted would have been the Logic Modules, but they include Module + specific role checks.

However, the Payment Processor does not and the PP_Streaming_v1.sol could be impacted by a malicious voter.

Inside PP_Streaming_v1.sol we have removeAllPaymentReceiverPayments which invokes _removePayment to remove a user's payment for the rest of the stream. This function is onlyOrchestratorAdmin. However the function cancelRunningPayments is onlyModule, which means our voted motion can call it directly. _cancelRunningOrders internally goes over all active payments and _removePayment's them.

This entire edge-case scenario would allow any single voter to cancel all payments at any time, unless the threshold of the voting contract gets changed. This can directly impact any Logic Module that implements a Streaming PP instead of a Simple PP, since the orders can be unexpectedly removed. For e.g the LM_PC_RecurringPayments_v1 implements an onlyOrchestratorAdmin function for removing payments, which we can bypass with the above scenario.

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

Recommendation:\ The options currently seen are 2:

1. Change the modifier of cancelRunningPayments from onlyModule to onlyOrchestratorAdmin
2. Change the onlyModule modifier to onlyModuleRole and add a specific role able to cancel payments. The same logic is applied throughout the LMs with roles like PAYMENT_PUSHER_ROLE, ASSERTER_ROLE, etc.

[0xfuje]

Hey, hope it's okay to comment, believe me or not I actually explored this possibility before submitting #67. The problem is it does not pass the validClient(address(client)) check since it enforces that the client passed in is the msg.sender as well, so in this case the voter contract, which would not have payments since it's not a client.

[PlamenTSV]

My bad, you are right

[PlamenTSV]

@0xfuje Hi, I just noticed another pattern, would like to hear an opinion:

1. Such an edge-case vote passes
2. The Voter module calls the Orchestrator's executeTxFromModule which is inherited from ModuleManagerBase_v1
3. The Orchestrator calls inside PP_Streaming_v1.sol and cancels the payments through removeAllPaymentReceiverPayments

[0xfuje]

@0xfuje Hi, I just noticed another pattern, would like to hear an opinion:

1. Such an edge-case vote passes
2. The Voter module calls the Orchestrator's executeTxFromModule which is inherited from ModuleManagerBase_v1
3. The Orchestrator calls inside PP_Streaming_v1.sol and cancels the payments through removeAllPaymentReceiverPayments

Hi, of course, I think I submitted the executeTxFromModule issue in #50.

In this case specially the removeAllPaymentReceiverPayments() is restricted by an onlyOrchestratorAdmin() which is the orchestrator's admin (or Authorizer's DEFAULT_ADMIN_ROLE) which is not the same as the orchestrator itself so the tx will fail.

I also think this may be intended functionality, but not too sure, since if a stream has to be cancelled, voters have a way to cancel it.

[PlamenTSV]

50 is reported on the assumption that a future module would be able to exploit it, but does not provide a scenario.

Custom modules that are not in the repo, aka the logic modules, are OOS, thus an owner who adds his own external module should be aware of the risks of code. The only plausible scenario is if an edge-case 0 threshold vote passes, which I highlight.

I am down for letting this be a duplicate, but it is more of an extension to #50.

[0xfuje]

https://github.com/hats-finance/Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb/issues/50 is reported on the assumption that a future module would be able to exploit it, but does not provide a scenario. Custom modules that are not in the repo, aka the logic modules, are OOS, thus an owner who adds his own external module should be aware of the risks of code.

Respectfully, I provided realistic scenarios when you asked me in: https://github.com/hats-finance/Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb/issues/50#issuecomment-2155869736, and we already discussed external modules in #50.

---

The only plausible scenario is if an edge-case 0 threshold vote passes, which I highlight. I am down for letting this be a duplicate, but it is more of an extension to https://github.com/hats-finance/Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb/issues/50.

As I said in my first comment above: https://github.com/hats-finance/Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb/issues/71#issuecomment-2157718762, this exploit does not work, because it does not pass the validClient() check.

---

If you are talking about the executeTxFromModule() scenario: https://github.com/hats-finance/Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb/issues/71#issuecomment-2158623911. It will revert because of the access control.

[0xfuje]

validClient() also forces the function to only cancel payments from the msg.sender aka the client that calls

[PlamenTSV]

My bad for not clarifying.

The valid attack path here is a combination:

1. If a 0 threshold voter passes a vote he can execute a transaction from the Voter module
2. From here he goes inside the Orchestrator and does executeTxFromModule
3. executeTxFromModule goes inside transferOrchestratorToken and transfers any amount out of the funding manager.

50 is valid, my bad and sorry for the confusion. The only thing this report adds is the above attack path.

[0xfuje]

My bad for not clarifying.

The valid attack path here is a combination:

1. If a 0 threshold voter passes a vote he can execute a transaction from the Voter module
2. From here he goes inside the Orchestrator and does executeTxFromModule
3. executeTxFromModule goes inside transferOrchestratorToken and transfers any amount out of the funding manager.

50 is valid, my bad and sorry for the confusion. The only thing this report adds is the above attack path.

Don't take offence, but isn't this a different vulnerability: the scenario I told you in: https://github.com/hats-finance/Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb/issues/50#issuecomment-2155869736? only adds https://github.com/hats-finance/Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb/issues/67 as a first step? I'm not saying it's a duplicate just to clarify.

[PlamenTSV]

My bad for not clarifying. The valid attack path here is a combination:

1. If a 0 threshold voter passes a vote he can execute a transaction from the Voter module
2. From here he goes inside the Orchestrator and does executeTxFromModule
3. executeTxFromModule goes inside transferOrchestratorToken and transfers any amount out of the funding manager.

50 is valid, my bad and sorry for the confusion. The only thing this report adds is the above attack path.

Don't take offence, but isn't this a different vulnerability: the scenario I told you in: #50 (comment)? only adds #67 as a first step? I'm not saying it's a duplicate just to clarify.

It's the combined steps in both. #50 mentions the scenario where the Voters do it, which is not plausible since it is a dedicated attack and the amount of voters needed is an assumtpion. However #67 shows an edge-case, which opens up the possibility of a single party draining the FM. It's the 2 steps combined yes.