Payment Split Contract Underflow Error

Smart Contracts
1

I have used the payment split contract as part of a bigger project which is currently undergoing audit.
The auditors have picked up the following in the Payment Split Contract Code which I wanted to verify with the Zeppelin community before I make changes.

Critical-2. Payment is calculated wrong in case a new payee is added. (Unresolved)
PaymentSplit.sol
In case, a new payee is added, pending payment for other users will be calculated wrong in case they have released tokens before.
Example:
User has a share, equal to 10. Total shares is 100.
User1 calls release(user1Address).
totalReceived = 100 ETH.
payment = 100 * 10 / 100 - 0.
_released(user1Address) = 10
_totalReleased = 10.
After that, another user is added with a share, equal to 10. total shares is 110.
User1 calls release again.
totalReceived = 105 ETH. (Additional ETH was transferred to contract).
payment = 105 * 10 / 110 - 10 = 9.54 - 10. An underflow is occured, preventing user from claiming. Future payment might also be corrupted.
Recommendation.
Recalculate shares for users, so that no underflow occur and payments are paid as intended

Any thoughts?

To give some context the contract should take in ETH and the split it between different 'share holders' depending on how many shares they own. It is one of the Open Zeppelin Finance contracts.
To claim their money a user calls the release function, at which point the amount they are owed is released to them based on what they have had before and their shareholding.

The full code is here.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/finance/PaymentSplitter.sol

3

imo its not very clear from your description what exactly is supposed to be calculated, what the expected results are and what the side effects from "release" are

4

Yes sorry I will update with the code, but for reference this is the code from the OpenZeppellin PaymentSplit.sol contract.

5

thanks for linking the contract, didnt know you were talking about a public one provided by OpenZeppelin.
have you modified the contract in your own project? the _addPayee method is private and only called in the constructor, so I am not sure how you are adding another user after the construction

6

PaymentSplitter from OpenZeppeling only allows you to add payee's on construction. The contract does not support adding payee's at a later stage.

Technicaly you might be able to add them, but as the auditor mentioned, the rewards will be incorrect if you do this.

7

Yeah thanks guys, I realised the same, I have modified it to allow adding additional payees which is causing the issue.
In the end I have added functionality to allowing adding new payees, but before any releases can happen the contract needs to be locked from adding new Payees. This allows me to deploy without the complete list of payees but also avoid the underflow error.