Make Gateway entry points nonreantrant

[alistair-singh]

Update to solidity 0.8.28 and Cancun EVM version. Make the entry points to the Gateway nonreantrant.

Resolves: https://linear.app/snowfork/issue/SNO-1222

[yrong]

Would it be better to use a custom slot as
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4c3ef87cf57b448a0b5fc68b8ce6604a31b60814/contracts/utils/ReentrancyGuardTransient.sol#L19 does?

[alistair-singh]

We can use that pattern. But in the V2 code we use slot 0? Is this an issue?

[alistair-singh]

It should be fine because all our variables except for DISPATCH_OVERHEAD_GAS are constant or immutable. DISPATCH_OVERHEAD_GAS is neither constant or immutable but we never write to it, so storage slot 0 is never used in Gateway.
Just to be safe I made DISPATCH_OVERHEAD_GAS constant in:
6148116
And added a storage slot for the reentrancy guard so that we do not mistakenly re-use that slot 0 in future by adding variables to the mutable Gateway.
266a350

Nice catch!

[vgeddes]

Transient storage lives in a separate data location from normal storage, at least that's what the internet says. So transient slot 0 is separate from normal storage slot 0. Can check this with a test.

I generally don't like the way OpenZeppelin code is implemented, its verbose.

[vgeddes]

Seems overly complicated compared to the version in Contracts V2, which is defined inline in the Gateway contract:

modifier nonreentrant {
    assembly {
        if tload(0) { revert(0, 0) }
        tstore(0, 1)
    }
    _;
    assembly {
        tstore(0, 0)
    }
}

[alistair-singh]

I do not think using slot 0 is the right thing to do even for V2. See this comment: #1358 (comment)

If someone adds a mutable variable to the Gateway contract at a later stage, is it not possible that the compiler will assign it to slot 0 and clash with the reentrancy guard? Or is my understanding of this incorrect?

So the only reason I moved this code to a library is because of the use of an explicit slot for reentrancy, but I can move the implementation with the explicit slot back to Gateway.sol?

[vgeddes]

Transient storage is a separate data location from normal storage. They don't overlap. So its not possible to overwrite normal storage. You check this with a test.

So yeah, am in favour of moving back to Gateway contract.

[alistair-singh]

Ah ok, so basically i misunderstood the storage slot.

Reverted in 4ec8b97

[yrong]

+1