Prysm: Potential issues due to granularity of timestamps

[banescusebi]

🐞 Bug Report

Description

The following requirement in the P2P specification can be violated:
"The block is not from a future slot (with a MAXIMUM_GOSSIP_CLOCK_DISPARITY allowance) -- i.e. validate that signed_beacon_block.message.slot <= current_slot"

There is an edge case scenario where this condition would be satisfied for a time that is actually in the future. The following is a code snippet from the VerifySlotTime function in beacon-chain/core/helpers/slot_epoch.go:

    slotTime := 1000 * (genesisTime + slot*params.BeaconConfig().SecondsPerSlot)
    currentTime := 1000 * uint64(roughtime.Now().Unix())
    tolerance := uint64(timeTolerance.Milliseconds()) // 500ms
    if slotTime > currentTime+tolerance { … }

Let’s say that slotTime is 5000s, and actual time represented by roughtime.Now() is 5900 ms. However, since roughtime.Now().Unix() returns time in seconds - that is, it returns 5s, and currentTime ends up being 5000 (same as slotTime). However, an actual current time is 5900s, which is 900ms higher than slotTime, but 900ms is bigger than 500ms. Therefore, the time difference is actually higher than MAXIMUM_GOSSIP_CLOCK_DISPARITY.

The same pattern is used in the validateAggregateAttTime function inside beacon-chain/sync/validate_aggregate_proof.go to verify if the attestation slot is within the last ATTESTATION_PROPAGATION_SLOT_RANGE slots.

Recommended fix/mitigation:

If tolerance is less than a second, it makes more sense to do time computations in milliseconds directly, rather than using seconds and then multiplying by 1000. Use https://golang.org/pkg/time/#Time.UnixNano instead of just .Unix(). Same applies to validating other things, e.g., attestations.

[JeevanPillay]

Hi, if this is still not finished, I would like to try and implement it.

I noticed that for beacon-chain/core/helpers/slot_epoch.go, @prestonvanloon had already implemented the fix, however, beacon-chain/sync/validate_aggregate_proof.go is yet to be implemented. I would follow a similar structure to the one implemented by @prestonvanloon (perhaps reuse the function he implemented if possible) and then some testing to ensure it works.

Cheers.

[nisdas]

Hey @lpfloyd ,

Thanks for your interest in this issue, however I already have a PR for this, will be pushing it up in a bit. If you are interested in looking at any other good issues, #5222 is also a good issue to take a look at. Thanks for taking a look !

[JeevanPillay]

Sure thing @nisdas! I'll have a look at #5222 when time permits.

[banescusebi]

Can the "audit" label be added to this issue pls?