PP_Streaming_v1.sol#cancelRunningPayments() - The function is uncallable by clients

[hats-bug-reporter[bot]]

Github username: -- Twitter username: @EgisSec Submission hash (on-chain): 0x3e6992ec2ca82992f51df0400c4b64d5dcae9b4e2d30b138dc5c6c928d6c8fa2 Severity: medium

Description: Description\ cancelRunningPayments has 2 modifiers, onlyModule and validClient.

 modifier onlyModule() {
        if (!orchestrator().isModule(_msgSender())) {
            revert Module__PaymentProcessor__OnlyCallableByModule();
        }
        _;
    }

modifier validClient(address client) {
        if (_msgSender() != client) {
            revert Module__PaymentProcessor__CannotCallOnOtherClientsOrders();
        }
        _;
    }

The modifiers check that only a module of the orchestrator can call the function and that msg.sender == client.

Modules that are supposed to call this function are any modules that inherit from ERC20PaymentClientBase_v1, since those are the only ones that interact with PP_Streaming_v1.

The issue is that neither ERC20PaymentClientBase_v1 or LM_PC_Bounties_v1, LM_PC_KPIRewarder_v1, LM_PC_PaymentRouter_v1, LM_PC_RecurringPayments_v1 and LM_PC_Staking_v1 have any way to call cancelRunningPaymentson PP_Streaming_v1, effectively making the function completely useless.

Attack Scenario\

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

Add a function to ERC20PaymentClientBase_v1 that exposes a way for the inheritors to call cancelRunningPayments.

[PlamenTSV]

It can be done when a successful vote is passed in the Voter contract, since it does a motion at a selected target with msg.sender = Voter module

This is the way I see it, could be wrong, leaving for further comment.

[PlamenTSV]

My comment is wrong, since the Voter contract is not a valid client. I see that the function in the current context does not serve a purpose, which imo is low. Will wait for mahdi or the team to take a look, it could be a part of a planned integration.

[FHieser]

Yes as it currently stands the function doesnt serve a purpose, as we dont have any Logicmodules that profit from that It is exposed via the IPaymentProcessor which can be accessed in the PaymentClients though.