Improper initialization of spending condition abstraction allows “v2 transactions” to exit using PaymentExitGame
`PaymentOutputToPaymentTxCondition` is an abstraction around the transaction signature check needed for many components of the exit games. Its only function, `verify`, returns `true` if one transaction (inputTxBytes) is spent by another transaction (spendingTxBytes):\\n```\\nfunction verify(\\n    bytes calldata inputTxBytes,\\n    uint16 outputIndex,\\n    uint256 inputTxPos,\\n    bytes calldata spendingTxBytes,\\n    uint16 inputIndex,\\n    bytes calldata signature,\\n    bytes calldata /\\*optionalArgs\\*/\\n)\\n    external\\n    view\\n    returns (bool)\\n{\\n    PaymentTransactionModel.Transaction memory inputTx = PaymentTransactionModel.decode(inputTxBytes);\\n    require(inputTx.txType == supportInputTxType, "Input tx is an unsupported payment tx type");\\n\\n    PaymentTransactionModel.Transaction memory spendingTx = PaymentTransactionModel.decode(spendingTxBytes);\\n    require(spendingTx.txType == supportSpendingTxType, "The spending tx is an unsupported payment tx type");\\n\\n    UtxoPosLib.UtxoPos memory utxoPos = UtxoPosLib.build(TxPosLib.TxPos(inputTxPos), outputIndex);\\n    require(\\n        spendingTx.inputs[inputIndex] == bytes32(utxoPos.value),\\n        "Spending tx points to the incorrect output UTXO position"\\n    );\\n\\n    address payable owner = inputTx.outputs[outputIndex].owner();\\n    require(owner == ECDSA.recover(eip712.hashTx(spendingTx), signature), "Tx in not signed correctly");\\n\\n    return true;\\n}\\n```\\n\\nVerification process\\nThe verification process is relatively straightforward. The contract performs some basic input validation, checking that the input transaction's `txType` matches `supportInputTxType`, and that the spending transaction's `txType` matches `supportSpendingTxType`. These values are set during construction.\\nNext, `verify` checks that the spending transaction contains an input that matches the position of one of the input transaction's outputs.\\nFinally, `verify` performs an EIP-712 hash on the spending transaction, and ensures it is signed by the owner of the output in question.\\nImplications of the abstraction\\nThe abstraction used requires several files to be visited to fully understand the function of each line of code: `ISpendingCondition`, `PaymentEIP712Lib`, `UtxoPosLib`, `TxPosLib`, `PaymentTransactionModel`, `PaymentOutputModel`, `RLPReader`, `ECDSA`, and `SpendingConditionRegistry`. Additionally, the abstraction obfuscates the underlying spending condition verification primitive where used.\\nFinally, understanding the abstraction requires an understanding of how `SpendingConditionRegistry` is initialized, as well as the nature of its relationship with `PlasmaFramework` and `ExitGameRegistry`. The aforementioned `txType` values, `supportInputTxType` and `supportSpendingTxType`, are set during construction. Their use in `ExitGameRegistry` seems to suggest they are intended to represent different versions of transaction types, and that separate exit game contracts are meant to handle different transaction types:\\n```\\n/\\*\\*\\n \\* @notice Registers an exit game within the PlasmaFramework. Only the maintainer can call the function.\\n \\* @dev Emits ExitGameRegistered event to notify clients\\n \\* @param \\_txType The tx type where the exit game wants to register\\n \\* @param \\_contract Address of the exit game contract\\n \\* @param \\_protocol The transaction protocol, either 1 for MVP or 2 for MoreVP\\n \\*/\\nfunction registerExitGame(uint256 \\_txType, address \\_contract, uint8 \\_protocol) public onlyFrom(getMaintainer()) {\\n    require(\\_txType != 0, "Should not register with tx type 0");\\n    require(\\_contract != address(0), "Should not register with an empty exit game address");\\n    require(\\_exitGames[\\_txType] == address(0), "The tx type is already registered");\\n    require(\\_exitGameToTxType[\\_contract] == 0, "The exit game contract is already registered");\\n    require(Protocol.isValidProtocol(\\_protocol), "Invalid protocol value");\\n\\n    \\_exitGames[\\_txType] = \\_contract;\\n    \\_exitGameToTxType[\\_contract] = \\_txType;\\n    \\_protocols[\\_txType] = \\_protocol;\\n    \\_exitGameQuarantine.quarantine(\\_contract);\\n\\n    emit ExitGameRegistered(\\_txType, \\_contract, \\_protocol);\\n}\\n```\\n\\nMigration and initialization\\nThe migration script seems to corroborate this interpretation:\\ncode/plasma_framework/migrations/5_deploy_and_register_payment_exit_game.js:L109-L124\\n```\\n// handle spending condition\\nawait deployer.deploy(\\n    PaymentOutputToPaymentTxCondition,\\n    plasmaFramework.address,\\n    PAYMENT\\_OUTPUT\\_TYPE,\\n    PAYMENT\\_TX\\_TYPE,\\n);\\nconst paymentToPaymentCondition = await PaymentOutputToPaymentTxCondition.deployed();\\n\\nawait deployer.deploy(\\n    PaymentOutputToPaymentTxCondition,\\n    plasmaFramework.address,\\n    PAYMENT\\_OUTPUT\\_TYPE,\\n    PAYMENT\\_V2\\_TX\\_TYPE,\\n);\\nconst paymentToPaymentV2Condition = await PaymentOutputToPaymentTxCondition.deployed();\\n```\\n\\nThe migration script shown above deploys two different versions of `PaymentOutputToPaymentTxCondition`. The first sets `supportInputTxType` and `supportSpendingTxType` to `PAYMENT_OUTPUT_TYPE` and `PAYMENT_TX_TYPE`, respectively. The second sets those same variables to `PAYMENT_OUTPUT_TYPE` and `PAYMENT_V2_TX_TYPE`, respectively.\\nThe migration script then registers both of these contracts in `SpendingConditionRegistry`, and then calls `renounceOwnership`, freezing the spending conditions registered permanently:\\ncode/plasma_framework/migrations/5_deploy_and_register_payment_exit_game.js:L126-L135\\n```\\nconsole.log(`Registering paymentToPaymentCondition (${paymentToPaymentCondition.address}) to spendingConditionRegistry`);\\nawait spendingConditionRegistry.registerSpendingCondition(\\n    PAYMENT\\_OUTPUT\\_TYPE, PAYMENT\\_TX\\_TYPE, paymentToPaymentCondition.address,\\n);\\n\\nconsole.log(`Registering paymentToPaymentV2Condition (${paymentToPaymentV2Condition.address}) to spendingConditionRegistry`);\\nawait spendingConditionRegistry.registerSpendingCondition(\\n    PAYMENT\\_OUTPUT\\_TYPE, PAYMENT\\_V2\\_TX\\_TYPE, paymentToPaymentV2Condition.address,\\n);\\nawait spendingConditionRegistry.renounceOwnership();\\n```\\n\\nFinally, the migration script registers a single exit game contract in PlasmaFramework:\\ncode/plasma_framework/migrations/5_deploy_and_register_payment_exit_game.js:L137-L143\\n```\\n// register the exit game to framework\\nawait plasmaFramework.registerExitGame(\\n    PAYMENT\\_TX\\_TYPE,\\n    paymentExitGame.address,\\n    config.frameworks.protocols.moreVp,\\n    { from: maintainerAddress },\\n);\\n```\\n\\nNote that the associated `_txType` is permanently associated with the deployed exit game contract:\\n```\\n/\\*\\*\\n \\* @notice Registers an exit game within the PlasmaFramework. Only the maintainer can call the function.\\n \\* @dev Emits ExitGameRegistered event to notify clients\\n \\* @param \\_txType The tx type where the exit game wants to register\\n \\* @param \\_contract Address of the exit game contract\\n \\* @param \\_protocol The transaction protocol, either 1 for MVP or 2 for MoreVP\\n \\*/\\nfunction registerExitGame(uint256 \\_txType, address \\_contract, uint8 \\_protocol) public onlyFrom(getMaintainer()) {\\n    require(\\_txType != 0, "Should not register with tx type 0");\\n    require(\\_contract != address(0), "Should not register with an empty exit game address");\\n    require(\\_exitGames[\\_txType] == address(0), "The tx type is already registered");\\n    require(\\_exitGameToTxType[\\_contract] == 0, "The exit game contract is already registered");\\n    require(Protocol.isValidProtocol(\\_protocol), "Invalid protocol value");\\n\\n    \\_exitGames[\\_txType] = \\_contract;\\n    \\_exitGameToTxType[\\_contract] = \\_txType;\\n    \\_protocols[\\_txType] = \\_protocol;\\n    \\_exitGameQuarantine.quarantine(\\_contract);\\n\\n    emit ExitGameRegistered(\\_txType, \\_contract, \\_protocol);\\n}\\n```\\n\\nConclusion\\nCrucially, this association is never used. It is implied heavily that transactions with some `txType` must use a certain registered exit game contract. In fact, this is not true. When using `PaymentExitGame`, its routers, and their associated controllers, the `txType` is invariably inferred from the encoded transaction, not from the mappings in `ExitGameRegistry`. If initialized as-is, both `PAYMENT_TX_TYPE` and `PAYMENT_V2_TX_TYPE` transactions may be exited using `PaymentExitGame`, provided they exist in the plasma chain.
Remove `PaymentOutputToPaymentTxCondition` and `SpendingConditionRegistry`\\nImplement checks for specific spending conditions directly in exit game controllers. Emphasize clarity of function: ensure it is clear when called from the top level that a signature verification check and spending condition check are being performed.\\nIf the inferred relationship between `txType` and `PaymentExitGame` is correct, ensure that each `PaymentExitGame` router checks for its supported `txType`. Alternatively, the check could be made in `PaymentExitGame` itself.\\nCorresponding issue in plasma-contracts repo: https://github.com/omisego/plasma-contracts/issues/472
```\\nfunction verify(\\n    bytes calldata inputTxBytes,\\n    uint16 outputIndex,\\n    uint256 inputTxPos,\\n    bytes calldata spendingTxBytes,\\n    uint16 inputIndex,\\n    bytes calldata signature,\\n    bytes calldata /\\*optionalArgs\\*/\\n)\\n    external\\n    view\\n    returns (bool)\\n{\\n    PaymentTransactionModel.Transaction memory inputTx = PaymentTransactionModel.decode(inputTxBytes);\\n    require(inputTx.txType == supportInputTxType, "Input tx is an unsupported payment tx type");\\n\\n    PaymentTransactionModel.Transaction memory spendingTx = PaymentTransactionModel.decode(spendingTxBytes);\\n    require(spendingTx.txType == supportSpendingTxType, "The spending tx is an unsupported payment tx type");\\n\\n    UtxoPosLib.UtxoPos memory utxoPos = UtxoPosLib.build(TxPosLib.TxPos(inputTxPos), outputIndex);\\n    require(\\n        spendingTx.inputs[inputIndex] == bytes32(utxoPos.value),\\n        "Spending tx points to the incorrect output UTXO position"\\n    );\\n\\n    address payable owner = inputTx.outputs[outputIndex].owner();\\n    require(owner == ECDSA.recover(eip712.hashTx(spendingTx), signature), "Tx in not signed correctly");\\n\\n    return true;\\n}\\n```\\n