Are there any Limits on #s of Payees in an OZ PaymentSplitter contract?

Support Contracts
1

General question about the PaymentSplitter contract. I use it all the time, but never for more than about 10 or 15 payees. Would it work well for a very large number of payees -- say 500+?? It seems to me that this would be inadvisable for various potential reason. But, I was just wondering if there are any recommended, technical, or practical limits for PaymentSplitter.

1 Like
2

The only practical issue is gas, which you can find by searching for any iterative code (for and while loops) in the contract.

If you do so, then you'll see that it appears only in one place in the code:

        for (uint256 i = 0; i < payees.length; i++) {
            _addPayee(payees[i], shares_[i]);
        }

Unfortunately, this code appears in the constructor of the contract, which means that you will not be able to deploy the contract with more than a certain number of payees, as the deployment transaction would cost more gas than the network's block-gas-limit.

And since you cannot add payees after the contract is deployed (no supporting functionality), you will not be able to serve more than that certain number of payees.

You might want to add a feature request for that (adding payees after the contract is deployed) on OpenZeppelin's GitHub.

P.S.:

Adding new payees after some payments have already been redeemed (by existing payees), would probably lead to all sorts of unfair payment scheme.

So any infrastructure supporting that will probably need to be restricted to a specific phase taking place after construction and before any payment has been redeemed.

1 Like
3

Yeah, I was thinking about the block-gas-limit as well. IDK how many payees I could get through, but I doubt it's > 500. I also, for this contract, like to do a global release function where I iterate through and release everyone's payments at once. So that would be another candidate for hitting a limit. I may do something entirely different here... As this is a one-off thing, I don't want to mess with adding payees ever, for various reasons even beyond any security concerns. Anyway, thanks for the thoughts there. :slight_smile:

1 Like
4

I'd agree with @barakman's response, my only concern would be the block's gasLimit

Doing some research in order to answer this question, I found this issue in Solidity. Not practical for purposes of the PaymentSplitter but leaving it here just for nerding.

5

@ernestognw : Thanks for commenting.

I cannot quite see how this issue is related to the question at hand in any way, as there are no dynamic memory allocations anywhere in the PaymentSplitter contract whatsoever (note that the _payees array is in storage, not in memory).

Let alone the fact that the issue itself has been resolved back in Solc 0.6.5, while this contract relies on Solc 0.8.0 or higher.

6

note that the _payees array is in storage, not in memory

Yes, but at the constructor, the payees array is "memory array of user-supplied length". Though, it'll only avoid deployment.

Nothing to worry about anyway. As you say, it's already fixed in recent Solidity version.

7

That's not a dynamic memory allocation within the contract code though, as explained in the description of the issue:

If you allocate dynamic memory arrays using new T[](length)...

To put it simply, there's no new in the code.

1 Like
8

To put it simply, there's no new in the code.

I ran a quick test and you're right. I didn't remember there's CALLDATACOPY.

For reference:

// SPDX-License-Identifier: MIT
pragma solidity 0.6.0;

contract Test {
    // Reference: https://blog.soliditylang.org/2020/04/06/memory-creation-overflow-bug/
    function shouldNotOutOfGas(uint256 length) public {
        uint[] memory x = new uint[](length);
        uint[] memory y = new uint[](4);
    }

    // Although it can't be realistically executed for size reasons, it will also
    // run out of gas for CALLDATACOPY's cost of memory expansion.
    function shouldOutOfGas(uint256[] memory array) public {}
}

The candidate one I saw in PaymentSplitter was in the constructor, but it'll also run out of gas.

Anyway, I think this:

And since you cannot add payees after the contract is deployed (no supporting functionality), you will not be able to serve more than that certain number of payees.

Might be for security reasons. It becomes difficult to handle security scenarios for adding new payees.

At least that might require discussion before implementing it. Is there any known reason for needing this?

9

Well, as I've mentioned in my original answer:

Adding new payees after some payments have already been redeemed (by existing payees), would probably lead to all sorts of unfair payment scheme.

So any infrastructure supporting that will probably need to be restricted to a specific phase taking place after construction and before any payment has been redeemed.

I suppose that on top of 'all sorts of unfair payment scheme', you could also add 'security issues'.

Again, as I've mentioned in my original answer:

And since you cannot add payees after the contract is deployed (no supporting functionality), you will not be able to serve more than that certain number of payees.

You might want to add a feature request for that (adding payees after the contract is deployed) on OpenZeppelin's GitHub.

The 'known reason for needing this' in the specific case at hand, is stated at the title of the question - this user is asking if the number of payees is limited, which it currently is, by block-gas-limit upon contract deployment. One way to solve it is by allowing to add payees after contract deployment.

10

I don't need this feature. But I'd second the security and payment concerns for anyone reading this. Obviously, you'd want to have such a feature (adding Payees post-deploy) locked down tightly. Any exploits there & hackers would drain contracts. But yeah, each payee gets a share. So, if people add payees, they will need to specify a share, which would affect all of the other shares by increasing the total shares. So, if Payee[0] gets 50 shares and Payee[1] gets 50, they each get 50% on deploy. If someone then adds Payee[3] with 50 shares, then each payee would be getting 33%. Add to that the complexity of scenarios if there is ETH in a contract already during the addPayee... and it could make for a lot of bad scenarios. Also, if block-gas-limit is an issue on deploy, then it could potentially be an issue if addPayee post-deploy wasn't limited because what if someone added, say, 500 wallets as payees? It might not be an issue if you required everyone to release their own funds, but if you wanted a function to iterate over all payees and pay out the lot at once, I guess that could cause issues, as well. So, there are many good reasons not to pursue this. Appreciate all of the thinking here, of course!

1 Like
11

Unlike the constructor, which is called only once, the addPayee function can be called several times, thus allowing a user to add those 500 wallets in several different transactions.

So no - the block gas limit, which is a "dead end" issue in the contract's constructor, is not a major blocker when it comes to any other contract function designated for adding payees.

The primary issues with adding payees dynamically, are financial and security related.

These are critical issues to consider of course, but they're not about gas limit or similar.

12

Right, but as I said, if someone wanted the payout to be a function that iterates over ALL payees, then you could be looking at a gas issue if there are 500 of them.

13

Well, if somebody wanted to add a new function which does whatever, then you could also be looking at a gas issue.

The question, as I understand it, is about the possible issues with having the PaymentSplitter contract (in its current version) serving 500 payees.

I have demonstrated a specific issue, and then showed how it could be addressed by extending the contract to support adding payees after deployment.

Now you're saying something like "well, if I also want to apply other changes in the contract, then there might be other gas issues".

Of course there might be other gas issues if you apply other changes, but that's true for every contract and every set of changes, regardless of this question.

14

Oh, I wasn't changing the scope of my question, which you fully answered immediately (thanks). I was merely pointing out, for anyone else who comes across this thread and is considering allowing _addPayee post-deploy, to keep in mind that they would now have unlimited payees, which could trickle-down into other problems later on. So, for example, for that hypothetical person, I might suggest placing a limit on the number of payees possible. Again, just a friendly reminder--not meant to be an extension of the original post.

1 Like