Analyze Panic error on some paymaster tx's

[rube-de]

investigated further what produces the panic error:

edit: the panic was because of the timestamp difference see #16 (comment)

[Copilot]

Pull request overview

This PR modifies the price feed staleness validation logic in the CrossChainPaymaster contract to prevent underflow reverts when oracle timestamps are in the future. While it addresses the underflow issue by adding a block.timestamp > tUpdated check, it inadvertently introduces a critical security vulnerability by allowing future timestamps to pass validation.

Key changes:

• Updated token price feed staleness check to use short-circuit evaluation to prevent underflow
• Updated ROSE price feed staleness check with the same pattern

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[matevz]

rUpdated and tUpdated can't be in the future. I'd revert immediately, if that's the case.

[rube-de]

rUpdated and tUpdated can't be in the future. I'd revert immediately, if that's the case.

edit: I was in my mind that block is from the block which is from the other chain, but true it's block from sapphire. 🤦‍♂️

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[rube-de]

after updating the contract with the future timestamp check. It is now clear the panic was because of it.
https://explorer.oasis.io/mainnet/sapphire/tx/db041bf1126b975725a48faa44eb44ada7da04dc9827f5aa4d7997eb6664b8d7

The relayer now retries if this happens, but this should probably be fixed in the price oracle.
In the price oracle right now time.time() is used instead of getting current block.timestamp, which leads that sometimes price observations are submitted with updatedAt 4sec ahead of block.timestamp. see e.g.:
https://explorer.oasis.io/mainnet/sapphire/tx/46b1ecf02990841b43583a3b52113b39b03a6ed692a9f3d0c45348a17ccec8aa

[matevz]

after updating the contract with the future timestamp check. It is now clear the panic was because of it. https://explorer.oasis.io/mainnet/sapphire/tx/db041bf1126b975725a48faa44eb44ada7da04dc9827f5aa4d7997eb6664b8d7

The relayer now retries if this happens, but this should probably be fixed in the price oracle. In the price oracle right now time.time() is used instead of getting current block.timestamp, which leads that sometimes price observations are submitted with updatedAt 4sec ahead of block.timestamp. see e.g.: https://explorer.oasis.io/mainnet/sapphire/tx/46b1ecf02990841b43583a3b52113b39b03a6ed692a9f3d0c45348a17ccec8aa

@kostko will have move insights, but I think the block.timestamp in Sapphire is the finalized time of the last block. So it's fine that updatedAt is ahead of the current's block timestamp. What puzzles me is that you should not catch the event and start processing it until the block was verified. Which is well after updatedAt.

The paymaster transactions are not triggered by the events, but can happen to be in the same block as the price feed updates. So updatedAt can be in the future.

[ptrus]

but I think the block.timestamp in Sapphire is the finalized time of the last block

yes