guhu95 - Downcasting in LibGameType.raw allows bypassing `respectedGameType` safety checks

[sherlock-admin3]

guhu95

high

Downcasting in LibGameType.raw allows bypassing respectedGameType safety checks

Summary

The LibGameType.raw function in the LibUDT.sol downcasts the GameType from uint32 to uint8. This downcast breaks the validation on games in multiple locations, bypassing respectedGameType restriction, which is a core safety mechanism, and enabling forged withdrawals attacks.

Vulnerability Detail

User defined type GameType uses underlying uint32, but LibGameType.raw downcasts it to uint8 retaining only the information in the least significant byte, resulting in incorrect conversion.

This method is used in several places to check that the GameType of a game matches an expected value:

• In OptimismPortal2.proveWithdrawalTransaction to validate against respectedGameType
• In OptimismPortal2.proveWithdrawalTransaction again to validate and oldGame against respectedGameType
• In OptimismPortal2.checkWithdrawal that's used by finalizeWithdrawalTransactionExternalProof to validate against respectedGameType
• In DisputeGameFactory.findLatestGames to match against input GameType.

For example, GameType.wrap(0), GameType.wrap(256), etc for 512, 768, 1024, 1280.. will all be considered equal in value in those locations.

Impact

Since respectedGameType is a key validation and safety check, and allows the bridge to fully trust a game contract's outputs, there are multiple severe outcomes. For example:

1. Prove withdrawals for no longer respected game type, bypassing the intended validation checks. For example, if respected game type was updated to 256, a game with game type 0 will pass all validation checks. However, game type 0 games, that were deprecated for a reason, may not allow correctly disputing fraudulent proposals. Alternatively, these games may not be monitored and disputed at all, due to the (incorrect) assumption that they can no longer be used for withdrawals - and so, undisputed, they will allow "proving" by default any fraudulent proposal and withdrawal.
2. Replay withdrawals between different OP Stack chains that use the same dispute factory, enabling withdrawal replay forgery. For example, if chain A uses game type 0, and chain B uses game type 256, games of each chain would be replayable, and allow proving withdrawals on the other chain.
3. Malicious withdrawals: A malicious Superchain DAO may request the addition of a game type for their own purposes into a common factory, using a malicious implementation. They can then exploit the vulnerability to prove malicious withdrawals from another OP Stack chain that uses the same factory. For example, they may request game type 256, which will allow forging withdrawals for a chain that uses game type 0.
4. Reproving can be disrupted if oldGame's type collides with newly respecteGameType.
5. findLatestGames will return irrelevant games as relevant, disrupting critical off-chain components (fault agents and front-ends).

Likelihood

• The same factory is built to manage multiple game types, as for example seen in the GameTypes (for types 0, 1, and 255) which are all configured in the same factory during OP Chain deployment . Additionally, if a common factory will be used by multiple OP Stack chains (similarly to the SuperchainConfig or ProtocolVersions) it will allow collisions not only between game types of the same chains, but also across different chains.
• The setImplementation function does not enforce sequential game type values, which makes "namespacing" via higher order bits very likely. This is already done with predeploy addresses (0x42...01), and with their implementation addresses (0xc0de...01) etc. This kind of namespacing will result in many exploitable collisions due to downcasting.

Code Snippet

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/dispute/lib/LibUDT.sol#L121C61-L121C76

POC

pragma solidity 0.8.15;

type GameType is uint32;

library LibGameType {
    function raw(GameType _gametype) internal pure returns (uint8 gametype_) {
        assembly {
            gametype_ := _gametype
        }
    }
}

using LibGameType for GameType global;

contract Test {
    function test() external {
        GameType t1 = GameType.wrap(uint32(256));
        GameType t2 = GameType.wrap(0);
        assert(t1.raw() == t2.raw());
    }
}

Tool used

Manual Review

Recommendation

The built in GameType.unwrap() function can be used to return the underlying built-in type instead of a custom unwrapping implementation. Alternatively, uint8 return value can be fixed to match uint32. However, it's worth noting that the manual casting approach, although simpler, is more error prone: this approach is likely what has caused this issue, since GameType was at some previous point uint8, and when it was updated to be of size uint32, this one spot was missed. Consequently, just relying on the built-in unwrap() is likely safer.

-library LibGameType {
- 
-   function raw(GameType _gametype) internal pure returns (uint8 gametype_) { 
-       assembly {  
-           gametype_ := _gametype  
-       }  
-}
...
-require(gameType.raw() == respectedGameType.raw(), "OptimismPortal: invalid game type");
+require(GameType.unwrap(gameType) == GameType.unwrap(respectedGameType), "OptimismPortal: invalid game type");
...
/// etc

Duplicate of #84

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/ethereum-optimism/optimism/pull/10152