feat: OptimismPortal wd invalidation mitigation

[smartcontracts]

Introduces a proposal that would prevent user withdrawals in the OptimismPortal from being invalidated any time that the fallback is utilized.

[ajsutton]

I'm in favour of this change. The validity checks are still really simple and it gives the guardian significantly more flexibility. The concern about user impact affects the guardian decision making process even if the actual impact winds up being small so providing better tools to simplify the decision making process seems well worth it to me.

[wildmolasses]

Looking at the impl, although checkWithdrawal no longer asserts against respectedGameType, we continue to assert against it in proveWithdrawalTransaction, implying the withdrawal GameType was once the respected one. If a GameType needs to be invalidated, the respectedGameType is changed and respectedGameTypeUpdatedAt is updated. This means another DisputeGame must be created, of the new respectedGameType and not the invalid GameType. 👍

[wildmolasses]

this variable could be decoupled from respectedGameType. Instead it could be called something like gameCreationValidFrom, and then the check would be like

require(
    createdAt >= gameCreationValidFrom,
    "OptimismPortal: dispute game created before latest creation validity timestamp"
);

what do you think?

the occurrences of respectedGameTypeUpdatedAt in the monorepo are low. sorry for the noob question but are there many concerns outside of the monorepo? perhaps integrators would be happy for the breaking change here as their withdrawal validity logic should be updated.

[smartcontracts]

Realistically I think you're correct here -- it's possible we could even keep the semantic meaning of respectedGameTypeUpdatedAt (as in, we do continue to update it when setRespectedGameType is called) but add this new variable gameCreationValidFrom with these new semantics.

[clabby]

This looks good to me. Simple enough to change up. Also will include some changes to the deputy guardian.

[smartcontracts]

Reminder to self: do not allow invalidation timestamp to be greater than current timestamp

[Ethnical]

An issue can possibly arise if the second case for the withdrawal invalidation without checking the respectGameType like in the past.

For example, this might be a possible attack vector:

1. The guardian make a tx to set the respectedGameType to 1 (permissioned game).
2. The attacker monitors the mempool and frontruns in the same block the call to setRespectGame(1).
3. In the same block, the attacker create a game using create() on the DisputeGameFactory contract. As, the createAt is a timestamp from the value block.timestamp, this will have the exact same value if we are before or after into the same block.
4. The attacker call proveWithdrawalTransaction() and then wait for the PROOF_MATURITY_DELAY_SECONDS submit the finalizeWithdrawalTransaction().
5. As the check (below in solidity) is using greater and equal than createdAt >= respectedGameTypeUpdatedAt the attacker might be able to finalize a transaction with the incorrect gameType.
6. The impact seems to be really low, but this would make an inconsistent state (as the optimismPortal and the Guardian that restricted the to certain gameType) the Guardian will think that only the certain gameType would be valid, for the next withdrawal and would invalid the other gameType but here the other gameType would be finalized with success at the block.timestamp would be the same).

if (disputeGameProxy.gameType().raw() != respectedGameType.raw()) //@audit: The current design review propose to not use this anymore?
    revert InvalidGameType();

require(
    createdAt >= respectedGameTypeUpdatedAt, //@audit: Usage of the >= instead of > is a potential issue here?
    "OptimismPortal: dispute game created before respected game type was updated"
);

This scenario might be invalid as maybe this is desired from the new design (still strange imo)?

[ajsutton]

Yeah would be worth making that a strict >. While that will mean that any withdrawals proven against the new game type after the change but in the same block are invalidated when they probably shouldn't be, that's a very unlikely corner case and much lower impact than having a withdrawal be unexpectedly valid.

[Ethnical]

Agreed, and let a comment but the rest looks a nice initiative to me!