search

movementlabsxyz / movement

The Movement Network is a Move-based L2 on Ethereum.
Apache License 2.0
50 stars 48 forks source link

MCR: Explain pre- and post-conditions around rolling over the epoch #499

Open mzabaluev opened 2 weeks ago

mzabaluev commented 2 weeks ago

Auditor: Ottersec Code: MCR Settlement Contract

Original filing

It’s possible for current epoch to go beyond yet uncommited block’s epoch assignments: Let’s assume there’s two commitments in queue: A(H=5, blockHeightEpochAssignments=10) and B(H=6, blockHeightEpochAssignments=10). The time based rollover in _acceptBlockCommitment here might roll over to the 11th epoch when accepting the first block, causing a Denial of Service for the contract since the epoch check here can never pass once this happens.

Scope of work

Accordingly to the team's evaluaution, there is no possibility to end up in an unreachable epoch with a correctly behaving supermajority, as explained in comment below.

The remaining work is in annotating the pre- and post-conditions for the relevant methods to facilitate audit and review.

mzabaluev commented 2 weeks ago

I'm not an expert on the MCR code, so correct me if I'm wrong.

The _acceptBlockCommitment function containing the rollOverEpoch call on line 228 is called on a commitment that gains a supermajority of stake. Any other commitments for the same height should not be accepted to get to the check on line 212, and furthermore such commitments should be slashed (which is not currently implemented). So it works as designed? Equivocation by attesters is prevented here.

l-monninger commented 1 week ago

Any other commitments for the same height should not be accepted to get to the check on line 212, and furthermore such commitments should be slashed (which is not currently implemented). So it works as designed?

I believe this is correct. But, what I had started doing and not completed which would help auditing and our review here is annotating the pre- and post-conditions for these methods. I can re-engage on this.

l-monninger commented 5 days ago

Marking this as decentralized because MCR does not need to be safe for centralized launch.