Skip to content
This repository has been archived by the owner on Feb 23, 2022. It is now read-only.

Lite client CheckSupport might return true in case h1 expired #57

Closed
milosevic opened this issue Nov 5, 2019 · 5 comments
Closed

Lite client CheckSupport might return true in case h1 expired #57

milosevic opened this issue Nov 5, 2019 · 5 comments
Labels
T:bug Type: Bug

Comments

@milosevic
Copy link
Contributor

If current time (now) is equal to if h1.Header.bfttime + tp - epsilon, where epsilon is very small value, then the if h1.Header.bfttime + tp > now so the first check of CheckSupport is true, but while we execute other checks (signature verification), trusted period of h1 expires. So it can happen that CheckSupport returns true and we trust h2 although validators from h1 are not anymore accountable. With carefully mounted attack faulty validators from h1 can craft h2 in whatever way they want and they will not be accountable for this attack.

@milosevic milosevic added the T:bug Type: Bug label Nov 5, 2019
@josef-widder
Copy link
Contributor

The main question for me is the relation of the trusting period and the unbonding time. I introduced the trusting period with the idea that trusting period << unbonding time. (Let's say tp = 2 weeks and unboding period = 3 weeks). I admit that this idea got lost in the write-up of the spec at some point.

To my current understanding, there are two (until now implicit) requirements:

(1) we want CheckSupport to return true as long as h2 was properly generated before the trusting period of h1 expired, that is, we want to exploit the guarantees of the tendermint failure model. For this, the current check is OK, because h1.Header.bfttime + tp > now is checked when we already locally have h2 at hand. So even if the check takes longer, there are sufficiently many validators of h1 correct by the time h2 was generated. So returning true is safe.

(2) if CheckSupport returns false due to misbehavior, we want sufficient time to report misbehavior of validators in h1. This is the concern raised by Zarko, and we didn't discuss this explicitly by now. In my opinion, we need
trusting period + time needed to check headers + time needed to report and punish misbehavior < unbonding period ... a lot of times in the picture, but I do not see a way around it.

@milosevic
Copy link
Contributor Author

Regarding 1) it seems that you are right as this attack is not possible if we are within Tendermint security model. But what about the case where we have more than 1/3 of faulty processes? It seems that this is a valid attack that can pass without being punished. I guess this is what you meant with 2).

@ebuchman
Copy link
Contributor

ebuchman commented Nov 5, 2019

My understanding is that tp << unbonding_period for this kind of reason.

More precisely, as Josef put it: trusting period + time needed to check headers + time needed to report and punish misbehavior < unbonding period. Of course, time needed to check headers should pale in comparison to the others

@cwgoes
Copy link

cwgoes commented Nov 5, 2019

I agree with @ebuchman, the extra time required for evidence submission should be much larger than the time required to check headers (the former should be on the order of days, and the latter on the order of minutes at worst, if network requests need to be made).

Still it might be prudent to state this assumption explicitly somewhere. We could even check the time after updating headers just to make sure it hasn't taken too long.

@ebuchman
Copy link
Contributor

Has this been sufficiently addressed?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T:bug Type: Bug
Projects
None yet
Development

No branches or pull requests

4 participants