Security Review #350: StreamingPaymentProcessor findings

[0xNuggan]

This issue is linked to

• PR #350

Explanation

• IStreamingPaymentProcessor uses an address as client for the claimForSpecificWalletId() function, while IPaymentProcessor uses an IERC20PaymentClient. Look into the reason for this inconsistency and make a decision
• In StreamingPaymentProcessor.sol, the _cancelRunningOrders() function uses an unbounded loop, which may run into out-of-gas errors. Reassess the situation and choose if we can do something about it.

Category:

• Information issue

Link to review comments

Should be marked as done

• [] https://github.com/InverterNetwork/inverter-contracts/pull/350#discussion_r1356389934
• [] https://github.com/InverterNetwork/inverter-contracts/pull/350#discussion_r1359795554

[FHieser]

1. Why here the IPaymentClient was removed but in IPaymentProcessor it was kept (in the new name as IERC20PaymentClient)? This is a question for @saxenism , becasue he made the changes for that. In it self this just has implications in the case we develop a different paymentClient that enables other tokens than ERC20. Do we want to hardcode the StreamingPaymentProcessor to only enable these or not? Personally I think locking this for current form is fine, because we dont know the implications of going with other token forms

[FHieser]

2. Here and in other places you use unbounded loops, which may run out of gas when you have a very large number of payment receivers (which in practice may not happen, but as contracts in theory can run for a very long time, it is hard to say). We have mentioned this in earlier reviews, feel free to ignore if you do not intend to fix.

This is a tough question to answer. In general this is obviously right and could hardlock some functionalities in the worst case, but were talking an unknown number of Orders here. I think we have 3 possible Solutions here:

1. Ignore it (definitely the easiest, but kinda douche bag solution here)
2. Dig a little into it and test on how many Orders it breaks and what the limits are here. If its some reasonable numbers go to solution 1 without the douche bag part
3. Change all relevant unbounded loops to loops that can be sectioned and a way to keep down the load

@saxenism do you have a opinion on this?