PullPayment.asyncSend() flow is unreliable

[sebastien-kr]

🎉 Description

Regarding contracts/payment/PullPayment.sol
You have to trust the person doing an asyncSend that he/she will not take out the amount that was promised as a payment before the withdrawal.

• 🐛 This is a bug report.

📝 Details

There should be some kind of insurance for the receiver that he/she will get the amount sent asynchronously.

Problem 1

It is possible to asyncSend the max value of uint256 to anyone, no check are done.
A simple solution would be to add this to the asyncSend method :

require(address(this).balance >= amount);

(Note : I know that would remove the ability to promise to withdraw as soon as there are funds later on.)

Problem 2

Using this flaw, let say you send 2 ETH to a and b. You only have 2.99 ETH, so when b withdraw first there is no problem. But when a tries to withdraw 2 ETH it fails because there is now only 1.99 ETH in the balance.
As a solution, this new require introduced earlier could check that the balance is greater than all the payments :

require(address(this).balance >= totalPayments);

Problem 3

It is possible to only withdraw 100% of the amount or nothing.
Shouldn't a be able to withdraw 1.99 at least ?

In PullPayment.sol line 23 :

uint256 payment = payments[payee];

This could be replaced with something like :

uint256 payment = address(this).balance.sub(payments[payee]);

Problem 4

There is no priority : first arrived, first served.
The solution may require a cancellation process to avoid freezing the balance with payments that are never pulled out / withdrawn... So I'll just skip a solution proposal.

---

I would like to add that I know we cannot be sure the balance will stay above a certain value so these constraints may be unnecessary work.
Still, the behavior of the asyncSend should be a bit more predictable or renamed to something like maySend to avoid confusion.

[frangio]

Thaks for the report @sebastien-kr. I agree with your assessment. Perhaps PullPayment should be a separate contract that holds the ether.

[Heph789]

PullPayment could be a separate contract whose owner is a different contract PullPaymentUser. Any money sent to PullPaymentUser is redirected to PullPayment if PullPayment's balance is less than totalPayments.

For example,

contract PullPayment is Ownable {
    function asyncSend() isOwner {}
}

contract PullPaymentUser {
    PullPayment vault;
    constructor() {
        vault = new PullPayment();
    }
    function () payable {
        if(address(vault).balance < vault.totalPayments) {
            uint256 amountNeeded = vault.totalPayments.sub(address(vault).balance);
            if (amountNeeded >= address(this).balance) {
                address(vault).transfer(address(this).balance);
            }
            else {
                address(vault).transfer(amountNeeded);
            }
        }
    }
}