Node/Gov: Don't enqueue already signed VAAs

[bruce-riley]

There is currently a problem where old Oasis transactions are being reobserved and they are flooding the governor. This is not really necessary because the transactions have most likely previously reached quorum.

This PR changes the governor to check to see if a transfer has been previously been approved, and if so, allows it to be published immediately, without impacting the notional value of the governor.

The following checks are done to determine if the transfer can be approved immediately:

• The VAA is already in the signed VAA database.
• The current digest matches what is in the database.
• The VAA in the database was signed by this guardian.

If the above conditions are met, the VAA can be approved immediately. Additionally, this check is made when pending transfers are reloaded on start up, which should immediately clean up the backlog when a guardian upgrades.

[tbjump]

Threat Surface

I agree with the issue that this change is addressing, but I would solve it differently. With the current approach we are adding

1. another code path that bypasses the governor
2. another place in the code where the guardian private key is being used

That's added threat surface in the two most security critical paths of the codebase, and I would like to avoid that.

I'd prefer if the processor would check if the Guardian had made this observation in the past and if so broadcast the existing signature. That way, the guardian private key doesn't get involved in this new code path at all.

False Negatives

The proposed change checking the stored VAA, which typically only has 13 signatures and even if Guardian g has signed a particular message in the past, g's signature may not be in the locally stored VAA for that message, leading to false negatives in this check. To address this issue, all observations would need to be stored, probably separately from the VAAs.

[evan-gray]

Let's try without the additional private key usage.

[evan-gray]

Right here I was expecting that we look through v.Signatures to find our index, then recover the public key from that signature (given that we just verified the hashes match above). If the public key matches our guardian's public key, we should be good to proceed. That would avoid needing the private key here, which would be best avoided.

[bruce-riley]

I like this approach much better! I have changed it to use Ecrecover with the public key rather than the private key.

[evan-gray]

Threat Surface

I agree with the issue that this change is addressing, but I would solve it differently. With the current approach we are adding

1. another code path that bypasses the governor
2. another place in the code where the guardian private key is being used

That's added threat surface in the two most security critical paths of the codebase, and I would like to avoid that.

I'd prefer if the processor would check if the Guardian had made this observation in the past and if so broadcast the existing signature. That way, the guardian private key doesn't get involved in this new code path at all.

False Negatives

The proposed change checking the stored VAA, which typically only has 13 signatures and even if Guardian g has signed a particular message in the past, g's signature may not be in the locally stored VAA for that message, leading to false negatives in this check. To address this issue, all observations would need to be stored, probably separately from the VAAs.

On the threat surface front, see my comment and perhaps that mitigates the concerns. Ultimately it would be good to bypass the re-observation route that could trigger this issue (and the need for this fix) altogether. That said, I think it important to mitigate the issue affecting production today and leave observation collecting (and re-observation handling) to a new, breakout service in the future.

[bruce-riley]

I agree with @evan-gray. I have changed the code to use Ecrecover with the public key rather than the private key. Also, we do not currently have a source of all observations that this guardian has signed, so the best we can do is compare against the signed VAAs. Yes, that could lead to some misses, but those misses are no worse than what we have now.

[tbjump]

I want to get more clarity on the problem this PR is addressing.
So right now if a Guardian g0 would re-sync their Oasis node it would lead to that Guardian re-observing many messages, which would exceed the governor limits, leading to many transactions getting enqueued in the Governor. But that shouldn't affect any other Guardians. If g0 has VAAs for those messages in store, he will not send any observation requests and the messages will just time-out. If on the other hand g0 does not have any VAA for this message stored, this PR will not change any behavior surrounding that message.

So this issue should only affect a the Guardians who are re-syncing their Oasis nodes and the issue goes away by itself ~48h after the re-sync is done, correct?

[bruce-riley]

This problem was solved differently, so this PR is no longer necessary.