SDK can't settle if partner closes with outdated balanceHash

[andrevmatos]

This is not expected on normal usage, but can be considered an attack vector from a malicious partner.
Upon closing a channel, partner needs to provide his perspective of the latest balanceProof we sent to them. We do the same for their balanceProof upon updateNonClosingBalanceProof on detecting their closing (if we're online) or MS stepping in. On settle though, the settling part needs to provide values (transferredAmount, lockedAmount, locksroot) which matches the hash sent by both sides (see raiden-network/raiden-contracts#1370 (comment)).

Since we currently only store our latest balanceProof (and theirs, but it doesn't matter here, since we called from our perspective), if they close with any past or zero balanceHash from us and don't settle, our SDK won't be able to settle and our deposit will be stuck in the channel forever.

Theoretically, a knowledgeable user can rebuild the matching balanceProof from transfers history and settle, but SDK doesn't do that currently. Notice the attacker can't force a random balanceHash to prevent us from settling ever again, as balanceHash and additionalHash are part of the signature, so they can only close with data we explicitly signed and sent to them.

The fix is to store all balanceProofs (both sent and received) on ChannelEnd, and look (backwards) for the matching balanceHash, and send these data on settle call. Optionally, we could have each end's balanceProof(s's list) dynamically [re]built from TransferState, as a view of it, instead of persisted on channel state.

Steps to Reproduce

1. Get partner to close channel with a past or zeroed balanceProof from us
2. After channel becomes settleable, try to settle it on LC's side

Expected Result

• We can always settle the channel if we didn't lose state

Actual Result

• Even with full state, on this case, channel becomes stuck at settling state, and trying to settle it throws an error on on-chain transaction

[kelsos]

So the solution to this issue would be to store all the balance proofs ever received for a channel?

[andrevmatos]

Yes, and search for right values (amounts, locksroot) when settling, or else settleChannel tx will fail.

[kelsos]

So being in ChannelEnd essentially means that all the received and send balance proofs will be stored in memory (and persisted in local storage)? We have to be careful with the implementation.

[andrevmatos]

Yes, but that's valid for the whole state anyway. All transfers and past channels are also kept there. The state is composed only of simple objects anyway, so I don't see it being relevant for some time, but you brought a valid point. Could you create an issue so we can evaluate risks and options about it? Thank you

[kelsos]

Sure :)

[andrevmatos]

I think we actually don't need to change state schema. We can simply iterate over the past transfers, and check the respective balanceProofs, as the info is already there. For partner's balanceHash mismatch, iterate over received transfers; shouldn't happen since we're the ones which sent the balanceHash for their end; for our balanceHash mismatch (if partner closed with an older balanceHash), iterate over sent transfers. On each TransferState, we check balanceHash from transfer[1]: Signed<LockedTransfer>, and either unlock[1]: Signed<Unlock> or lockExpired[1]: Signed<LockExpired>, which are the messages containing balanceProofs.
We can do this iteration only if hashes didn't match for latest ends (which is the common case). We should get rid of transferClear, which we never exposed anyway, and assume we always have access to the full transfer history. We may iterate in transfer's reverse order, as later BPs should have higher probability of being right (in case of some dessync). Easier than we thought.

[kelsos]

That sounds more reasonable, especially if we already have all the data in the state.