fix(balance)

[matdehaast]

Currently there is a serious bug in the balance logic due to the way minimum balance works. Packets will be forwarded to an outgoing peer and only on the fulfil will we check if the balance exceeds the minimum for that peer (in the subtract logic). If it does we then throw an error and reject the packet downstream. However we have already sent the prepare and received a fulfil and have thus lost money as we owe our outgoing peer.

The solution proposed though changes the balance to be symmetrically tracked which I know @justmoon said shouldn't be necessary. Though I am not sure how else you enforce the minimum criteria without atomically changing the balance before forwarding it to your outgoing peer.

[kincaidoneil]

and have thus lost money as we owe our outgoing peer.

You haven't lost money, but the peer will, since you think you owe them less than they think you owe them.

I don't think this a problem, since in most cases minimum balances aren't very useful (the default of -Infinity is fine for most). If the peer is prefunding you some massive amount of money such that it goes below your minimum, that's kinda on them (but again, seems pretty rare).

---

With respect to the fix itself, this behavior opens up a serious security issue. By preemptively subtracting the balance before the packet is fulfilled, a peer can steal money before the packet is rejected: it just needs you to trigger a settlement during that window.

Exploiting it would be pretty trivial:

1. Send an unfulfillable packet to yourself for 1 unit.
2. Send another packet to yourself for 1 unit, and fulfill it.
3. Connector triggers a settlement to you for 2 units (depending upon the settleTo/threshold config, but that's still trivial to game by just fulfilling more packets).
4. Original packet gets rejected due to a timeout (or you could reject it), and #profit 1 unit.

To solve the original minimum balance issue, you would need to separately track a pending, in flight balance and use that on outgoing Prepares to check if it goes below the minimum, rather than modifying the balance itself. However, I don't think this is worth the effort for such a minor issue.

[adrianhopebailie]

You haven't lost money, but the peer will, since you think you owe them less than they think you owe them.

If I owe you money (I sent you a prepare and you fulfilled it) but after the fact I decide to reject that packet, who has "lost money"?

I think it's me since I owe you that money.

I don't think this a problem, since in most cases minimum balances aren't very useful (the default of -Infinity is fine for most).

Let's not look at this through too narrow a use case lens.

In the majority of cases on a real-money network two peers are going to have a legal contract between them that says owed amounts are legally enforceable. So every packet I forward that fits this profile is me increasing my losses.

The minimum balance is something I use to ensure I never owe you more than I am comfortable with. It's a trip wire that I would imagine most connectors setting to something other than -Infinity.

[adrianhopebailie]

@kincaidoneil the exploit you describe just highlights for me the need to track balances better (as you point out).

I don't agree that it's not worth the effort. As @BobWay has pointed out to us before with SNAP this is the correct way to do it and we should just do it like that in the connector.

Then, if you want to have a simple accounting system and track balances as a single value you can do that but that's not the standard behaviour.

[kincaidoneil]

If I owe you money (I sent you a prepare and you fulfilled it) but after the fact I decide to reject that packet, who has "lost money"?

In the majority of cases on a real-money network two peers are going to have a legal contract between them that says owed amounts are legally enforceable. So every packet I forward that fits this profile is me increasing my losses.

Fair enough

I'd just like to underscore this is definitely an edge case, or else something is likely configured wrong. For example, as long as the delta between the settleThreshold and minimum is greater than the maxPacketAmount, this case should almost never happen, since settlement would be triggered before it even gets close to the minimum. Therefore, this issue really only affects prefunded relationships, which are probably less applicable to legal contract scenario you're describing. And it's a super edge case for even for those: your peer would have to prefund you between < abs(minimum) but > abs(minimum) - maxPacketAmount, and then a single packet would have to be forwarded and fulfilled that puts the balance below the minimum. Which, too bad for them ¯\(ツ)/¯

(edit: in the case of automated settlement, but if the settlement is manual that makes more sense)

[matdehaast]

@kincaidoneil thanks for pointing out the issue above. I suspected it was more nuanced and was a reason for the initial way of doing it.

You mention it as an edge case for automated settlement. But would this case not occur if your SE goes into an exception state and it doesn't accept settlement messages?

[sharafian]

If we subtract on an outgoing packet and then add on a reject, it could put us above our maximum balance.

Imagine my max balance is 100, current balance is 80.

1. I send out a packet for 30. Now my balance is 50 because outgoing packet was subtracted on prepare.
2. I get an incoming packet for 40. Now my balance is 90 because incoming packets are added.
3. Outgoing packet for 30 is rejected, meaning that it's added back to the balance. Now our balance is 120 which is greater than our maximum.

The only correct way I can think of is to track two balances, one that represents your minimum balance possible and one that represents your maximum balance possible. Outgoing packets are counted against min-possible but incoming packets are not added. Incoming packets are counted against max-possible but outgoing packets are not subtracted. min-possible can never exceed min balance and max-possible can never exceed max balance.