OmiseGo MoreVP

Description

checkMembership is used by several contracts to prove that transactions exist in the child chain. The function uses a leaf, an index, and a proof to construct a hypothetical root hash. This constructed hash is compared to the passed in rootHash parameter. If the two are equivalent, the proof is considered valid.

The proof is performed iteratively, and uses a pseudo-index (j) to determine whether the next proof element represents a “left branch” or “right branch”:

code/plasma_framework/contracts/src/utils/Merkle.sol:L28-L41

uint256 j = index;
// Note: We're skipping the first 32 bytes of `proof`, which holds the size of the dynamically sized `bytes`
for (uint256 i = 32; i <= proof.length; i += 32) {
    // solhint-disable-next-line no-inline-assembly
    assembly {
        proofElement := mload(add(proof, i))
    }
    if (j % 2 == 0) {
        computedHash = keccak256(abi.encodePacked(NODE_SALT, computedHash, proofElement));
    } else {
        computedHash = keccak256(abi.encodePacked(NODE_SALT, proofElement, computedHash));
    }
    j = j / 2;
}

If j is even, the computed hash is placed before the next proof element. If j is odd, the computed hash is placed after the next proof element. After each iteration, j is decremented by j = j / 2.

Because checkMembership makes no requirements on the height of the tree or the size of the proof relative to the provided index, it is possible to pass in invalid values for index that prove a leaf’s existence in multiple locations in the tree.

Examples

By modifying existing tests, we showed that for a tree with 3 leaves, leaf 2 can be proven to exist at indices 2, 6, and 10 using the same proof each time. The modified test can be found here: https://gist.github.com/wadeAlexC/01b60099282a026f8dc1ac85d83489fd#file-merkle-test-js-L40-L67

it('should accidentally allow different indices to use the same proof', async () => {
  const rootHash = this.merkleTree.root;
  const proof = this.merkleTree.getInclusionProof(leaves[2]);

  const result = await this.merkleContract.checkMembership(
    leaves[2],
    2,
    rootHash,
    proof,
  );
  expect(result).to.be.true;

  const nextResult = await this.merkleContract.checkMembership(
    leaves[2],
    6,
    rootHash,
    proof,
  );
  expect(nextResult).to.be.true;

  const nextNextResult = await this.merkleContract.checkMembership(
    leaves[2],
    10,
    rootHash,
    proof,
  );
  expect(nextNextResult).to.be.true;
});

Conclusion

Exit processing is meant to bypass exits processed more than once. This is implemented using an “output id” system, where each exited output should correspond to a unique id that gets flagged in the ExitGameController contract as it’s exited. Before an exit is processed, its output id is calculated and checked against ExitGameController. If the output has already been exited, the exit being processed is deleted and skipped. Crucially, output id is calculated differently for standard transactions and deposit transactions: deposit output ids factor in the transaction index.

By using the behavior described in this issue in conjunction with methods discussed in issue 5.8 and issue 5.10, we showed that deposit transactions can be exited twice using indices 0 and 2**16. Because of the distinct output id calculation, these exits have different output ids and can be processed twice, allowing users to exit double their deposited amount.

A modified StandardExit.load.test.js shows that exits are successfully enqueued with a transaction index of 65536: https://gist.github.com/wadeAlexC/4ad459b7510e512bc9556e7c919e0965#file-standardexit-load-test-js-L55

Recommendation

Use the length of the proof to determine the maximum allowed index. The passed-in index should satisfy the following criterion: index < 2**(proof.length/32). Additionally, ensure range checks on transaction position decoding are sufficiently restrictive (see https://github.com/ConsenSys/omisego-morevp-audit-2019-10/issues/20).

Corresponding issue in plasma-contracts repo: https://github.com/omisego/plasma-contracts/issues/546

Description

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):

code/plasma_framework/contracts/src/exits/payment/spendingConditions/PaymentOutputToPaymentTxCondition.sol:L40-L69

function verify(
    bytes calldata inputTxBytes,
    uint16 outputIndex,
    uint256 inputTxPos,
    bytes calldata spendingTxBytes,
    uint16 inputIndex,
    bytes calldata signature,
    bytes calldata /*optionalArgs*/
)
    external
    view
    returns (bool)
{
    PaymentTransactionModel.Transaction memory inputTx = PaymentTransactionModel.decode(inputTxBytes);
    require(inputTx.txType == supportInputTxType, "Input tx is an unsupported payment tx type");

    PaymentTransactionModel.Transaction memory spendingTx = PaymentTransactionModel.decode(spendingTxBytes);
    require(spendingTx.txType == supportSpendingTxType, "The spending tx is an unsupported payment tx type");

    UtxoPosLib.UtxoPos memory utxoPos = UtxoPosLib.build(TxPosLib.TxPos(inputTxPos), outputIndex);
    require(
        spendingTx.inputs[inputIndex] == bytes32(utxoPos.value),
        "Spending tx points to the incorrect output UTXO position"
    );

    address payable owner = inputTx.outputs[outputIndex].owner();
    require(owner == ECDSA.recover(eip712.hashTx(spendingTx), signature), "Tx in not signed correctly");

    return true;
}

Verification process

The 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.

Next, verify checks that the spending transaction contains an input that matches the position of one of the input transaction’s outputs.

Finally, verify performs an EIP-712 hash on the spending transaction, and ensures it is signed by the owner of the output in question.

Implications of the abstraction

The 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.

Finally, 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:

code/plasma_framework/contracts/src/framework/registries/ExitGameRegistry.sol:L58-L78

/**
 * @notice Registers an exit game within the PlasmaFramework. Only the maintainer can call the function.
 * @dev Emits ExitGameRegistered event to notify clients
 * @param _txType The tx type where the exit game wants to register
 * @param _contract Address of the exit game contract
 * @param _protocol The transaction protocol, either 1 for MVP or 2 for MoreVP
 */
function registerExitGame(uint256 _txType, address _contract, uint8 _protocol) public onlyFrom(getMaintainer()) {
    require(_txType != 0, "Should not register with tx type 0");
    require(_contract != address(0), "Should not register with an empty exit game address");
    require(_exitGames[_txType] == address(0), "The tx type is already registered");
    require(_exitGameToTxType[_contract] == 0, "The exit game contract is already registered");
    require(Protocol.isValidProtocol(_protocol), "Invalid protocol value");

    _exitGames[_txType] = _contract;
    _exitGameToTxType[_contract] = _txType;
    _protocols[_txType] = _protocol;
    _exitGameQuarantine.quarantine(_contract);

    emit ExitGameRegistered(_txType, _contract, _protocol);
}

Migration and initialization

The migration script seems to corroborate this interpretation:

code/plasma_framework/migrations/5_deploy_and_register_payment_exit_game.js:L109-L124

// handle spending condition
await deployer.deploy(
    PaymentOutputToPaymentTxCondition,
    plasmaFramework.address,
    PAYMENT_OUTPUT_TYPE,
    PAYMENT_TX_TYPE,
);
const paymentToPaymentCondition = await PaymentOutputToPaymentTxCondition.deployed();

await deployer.deploy(
    PaymentOutputToPaymentTxCondition,
    plasmaFramework.address,
    PAYMENT_OUTPUT_TYPE,
    PAYMENT_V2_TX_TYPE,
);
const paymentToPaymentV2Condition = await PaymentOutputToPaymentTxCondition.deployed();

The 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.

The migration script then registers both of these contracts in SpendingConditionRegistry, and then calls renounceOwnership, freezing the spending conditions registered permanently:

code/plasma_framework/migrations/5_deploy_and_register_payment_exit_game.js:L126-L135

console.log(`Registering paymentToPaymentCondition (${paymentToPaymentCondition.address}) to spendingConditionRegistry`);
await spendingConditionRegistry.registerSpendingCondition(
    PAYMENT_OUTPUT_TYPE, PAYMENT_TX_TYPE, paymentToPaymentCondition.address,
);

console.log(`Registering paymentToPaymentV2Condition (${paymentToPaymentV2Condition.address}) to spendingConditionRegistry`);
await spendingConditionRegistry.registerSpendingCondition(
    PAYMENT_OUTPUT_TYPE, PAYMENT_V2_TX_TYPE, paymentToPaymentV2Condition.address,
);
await spendingConditionRegistry.renounceOwnership();

Finally, the migration script registers a single exit game contract in PlasmaFramework:

code/plasma_framework/migrations/5_deploy_and_register_payment_exit_game.js:L137-L143

// register the exit game to framework
await plasmaFramework.registerExitGame(
    PAYMENT_TX_TYPE,
    paymentExitGame.address,
    config.frameworks.protocols.moreVp,
    { from: maintainerAddress },
);

Note that the associated _txType is permanently associated with the deployed exit game contract:

code/plasma_framework/contracts/src/framework/registries/ExitGameRegistry.sol:L58-L78

/**
 * @notice Registers an exit game within the PlasmaFramework. Only the maintainer can call the function.
 * @dev Emits ExitGameRegistered event to notify clients
 * @param _txType The tx type where the exit game wants to register
 * @param _contract Address of the exit game contract
 * @param _protocol The transaction protocol, either 1 for MVP or 2 for MoreVP
 */
function registerExitGame(uint256 _txType, address _contract, uint8 _protocol) public onlyFrom(getMaintainer()) {
    require(_txType != 0, "Should not register with tx type 0");
    require(_contract != address(0), "Should not register with an empty exit game address");
    require(_exitGames[_txType] == address(0), "The tx type is already registered");
    require(_exitGameToTxType[_contract] == 0, "The exit game contract is already registered");
    require(Protocol.isValidProtocol(_protocol), "Invalid protocol value");

    _exitGames[_txType] = _contract;
    _exitGameToTxType[_contract] = _txType;
    _protocols[_txType] = _protocol;
    _exitGameQuarantine.quarantine(_contract);

    emit ExitGameRegistered(_txType, _contract, _protocol);
}

Conclusion

Crucially, 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.

Recommendation

• Remove PaymentOutputToPaymentTxCondition and SpendingConditionRegistry

• Implement 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.

• If 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.

Corresponding issue in plasma-contracts repo: https://github.com/omisego/plasma-contracts/issues/472

Description

The current implementation of RLP decoding can take 2 different txBytes and decode them to the same structure. Specifically, the RLPReader.toUint method can decode 2 different types of bytes to the same number. For example:

• 0x821234 is decoded to uint(0x1234)
• 0x83001234 is decoded to uint(0x1234)
• 0xc101 can decode to uint(1), even though the tag specifies a short list
• 0x01 can decode to uint(1), even though the tag specifies a single byte

As explanation for this encoding:

0x821234 is broken down into 2 parts:

• 0x82 - represents 0x80 (the string tag) + 0x02 bytes encoded
• 0x1234 - are the encoded bytes

The same for 0x83001234:

• 0x83 - represents 0x80 (the string tag) + 0x03 bytes encoded
• 0x001234 - are the encoded bytes

The current implementation casts the encoded bytes into a uint256, so these different encodings are interpreted by the contracts as the same number:

uint(0x1234) = uint(0x001234)

code/plasma_framework/contracts/src/utils/RLPReader.sol:L112

result := mload(memPtr)

Having different valid encodings for the same data is a problem because the encodings are used to create hashes that are used as unique ids. This means that multiple ids can be created for the same data. The data should only have one possible id.

The encoding is used to create ids in these parts of the code:

• Outputid.sol

code/plasma_framework/contracts/src/exits/utils/OutputId.sol:L18

return keccak256(abi.encodePacked(_txBytes, _outputIndex, _utxoPosValue));

code/plasma_framework/contracts/src/exits/utils/OutputId.sol:L32

return keccak256(abi.encodePacked(_txBytes, _outputIndex));

• ExitId.sol

code/plasma_framework/contracts/src/exits/utils/ExitId.sol:L41

bytes32 hashData = keccak256(abi.encodePacked(_txBytes, _utxoPos.value));

code/plasma_framework/contracts/src/exits/utils/ExitId.sol:L54

return uint160((uint256(keccak256(_txBytes)) >> 105).setBit(151));

• TxFinalizationVerifier.sol

code/plasma_framework/contracts/src/exits/utils/TxFinalizationVerifier.sol:L55

bytes32 leafData = keccak256(data.txBytes);

Other methods that are affected because they rely on the return values of these methods:

• ExitId.sol
  • getStandardExitId
  • getInFlightExitId
• OutputId.sol
  • computeDepositOutputId
  • computeNormalOutputId
• PaymentChallengeIFENotCanonical.sol
  • verifyAndDeterminePositionOfTransactionIncludedInBlock
  • verifyCompetingTxFinalized
• PaymentChallengeStandardExit.sol
  • verifyChallengeTxProtocolFinalized
• PaymentStartInFlightExit.sol
  • verifyInputTransactionIsStandardFinalized
• PaymentExitGame.sol
  • getStandardExitId
  • getInFlightExitId
• PaymentOutputToPaymentTxCondition.sol
  • verify

Recommendation

Enforce strict-length decoding for txBytes, and specify that uint is decoded from a 32-byte short string.

Enforcing a 32-byte length for uint means that 0x1234 should always be encoded as:

0xa00000000000000000000000000000000000000000000000000000000000001234

• 0xa0 represents the tag + the length: 0x80 + 32
• 0000000000000000000000000000000000000000000000000000000000001234 is the number 32 bytes long with leading zeroes

Unfortunately, using leading zeroes is against the RLP spec:

https://github.com/ethereum/wiki/wiki/RLP

positive RLP integers must be represented in big endian binary form with no leading zeroes

This means that libraries interacting with OMG contracts which are going to correctly and fully implement the spec will generate “incorrect” encodings for uints; encodings that are not going to be recognized by the OMG contracts.

Fully correct spec encoding: 0x821234. Proposed encoding in this solution: 0xa00000000000000000000000000000000000000000000000000000000000001234.

Similarly enforce restrictions where they can be added; this is possible because of the strict structure format that needs to be encoded.

Some other potential solutions are included below. Note that these solutions are not recommended for reasons included below:

1. Normalize the encoding that gets passed to methods that hash the transaction for use as an id:

This can be implemented in the methods that call keccak256 on txBytes and should decode and re-encode the passed txBytes in order to normalize the passed encoding.

• a txBytes is passed
• the txBytes are decoded into structure: tmpDecodedStruct = decode(txBytes)
• the tmpDecodedStruct is re-encoded in order to normalize it: normalizedTxBytes = encode(txBytes)

This method is not recommended because it needs a Solidity encoder to be implemented and a lot of gas will be used to decode and re-encode the initial txBytes.

2. Correctly and fully implement RLP decoding

This is another solution that adds a lot of code and is prone to errors.

The solution would be to enforce all of the restrictions when decoding and not accept any encoding that doesn’t fully follow the spec. This for example means that is should not accept uints with leading zeroes.

This is a problem because it needs a lot of code that is not easy to write in Solidity (or EVM).

Description

TxFinalizationVerifier is an abstraction around the block inclusion check needed for many of the features of plasma exit games. It uses a struct defined in TxFinalizationModel as inputs to its two functions: isStandardFinalized and isProtocolFinalized.

isStandardFinalized returns the result of an inclusion proof. Although there are several branches, only the first is used:

code/plasma_framework/contracts/src/exits/utils/TxFinalizationVerifier.sol:L19-L32

/**
* @notice Checks whether a transaction is "standard finalized"
* @dev MVP: requires that both inclusion proof and confirm signature is checked
* @dev MoreVp: checks inclusion proof only
*/
function isStandardFinalized(Model.Data memory data) public view returns (bool) {
    if (data.protocol == Protocol.MORE_VP()) {
        return checkInclusionProof(data);
    } else if (data.protocol == Protocol.MVP()) {
        revert("MVP is not yet supported");
    } else {
        revert("Invalid protocol value");
    }
}

isProtocolFinalized is unused:

code/plasma_framework/contracts/src/exits/utils/TxFinalizationVerifier.sol:L34-L47

/**
* @notice Checks whether a transaction is "protocol finalized"
* @dev MVP: must be standard finalized
* @dev MoreVp: allows in-flight tx, so only checks for the existence of the transaction
*/
function isProtocolFinalized(Model.Data memory data) public view returns (bool) {
    if (data.protocol == Protocol.MORE_VP()) {
        return data.txBytes.length > 0;
    } else if (data.protocol == Protocol.MVP()) {
        revert("MVP is not yet supported");
    } else {
        revert("Invalid protocol value");
    }
}

The abstraction used introduces branching logic and requires several files to be visited to fully understand the function of each line of code: ITxFinalizationVerifier, TxFinalizationModel, TxPosLib, Protocol, BlockController, and Merkle. Additionally, the abstraction obfuscates the underlying inclusion proof primitive when used in the exit game contracts. isStandardFinalized is not clearly an inclusion proof, and isProtocolFinalized simply adds confusion.

Finally, the abstraction may have ramifications on the safety of Merkle.sol. As it stands now, Merkle.checkMembership should never be called directly by the exit game controllers, as it lacks an important check made in TxFinalizationVerifier.checkInclusionProof:

code/plasma_framework/contracts/src/exits/utils/TxFinalizationVerifier.sol:L49-L59

function checkInclusionProof(Model.Data memory data) private view returns (bool) {
    if (data.inclusionProof.length == 0) {
        return false;
    }

    (bytes32 root,) = data.framework.blocks(data.txPos.blockNum());
    bytes32 leafData = keccak256(data.txBytes);
    return Merkle.checkMembership(
        leafData, data.txPos.txIndex(), root, data.inclusionProof
    );
}

By introducing the abstraction of TxFinalizationVerifier, the input validation performed by Merkle is split across multiple files, and the reasonable-seeming decision of calling Merkle.checkMembership directly becomes unsafe. In fact, this occurs in one location in the contracts:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFENotCanonical.sol:L187-L204

function verifyAndDeterminePositionOfTransactionIncludedInBlock(
    bytes memory txbytes,
    UtxoPosLib.UtxoPos memory utxoPos,
    bytes32 root,
    bytes memory inclusionProof
)
    private
    pure
    returns(uint256)
{
    bytes32 leaf = keccak256(txbytes);
    require(
        Merkle.checkMembership(leaf, utxoPos.txIndex(), root, inclusionProof),
        "Transaction is not included in block of Plasma chain"
    );

    return utxoPos.value;
}

Recommendation

1. Remove TxFinalizationVerifier and TxFinalizationModel

2. Implement a proof length check in Merkle.sol

3. Call Merkle.checkMembership directly from exit controller contracts:

• PaymentChallengeIFEOutputSpent.verifyInFlightTransactionStandardFinalized:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEOutputSpent.sol:L91

require(controller.txFinalizationVerifier.isStandardFinalized(finalizationData), "In-flight transaction not finalized");

• PaymentChallengeIFENotCanonical.verifyCompetingTxFinalized:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFENotCanonical.sol:L244

require(self.txFinalizationVerifier.isStandardFinalized(finalizationData), "Failed to verify the position of competing tx");

• PaymentStartInFlightExit.verifyInputTransactionIsStandardFinalized:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentStartInFlightExit.sol:L307-L308

require(exitData.controller.txFinalizationVerifier.isStandardFinalized(finalizationData),
        "Input transaction is not standard finalized");

4. If none of the above recommendations are implemented, ensure that PaymentChallengeIFENotCanonical uses the abstraction TxFinalizationVerifier so that a length check is performed on the inclusion proof.

Corresponding issue in plasma-contracts repo: https://github.com/omisego/plasma-contracts/issues/471

Description

A observation with the current Merkle tree implementation is that it may be possible to validate nodes other than leaves. This is done by providing checkMembership with a reference to a hash within the tree, rather than a leaf.

code/plasma_framework/contracts/src/utils/Merkle.sol:L9-L42

/**
 * @notice Checks that a leaf hash is contained in a root hash
 * @param leaf Leaf hash to verify
 * @param index Position of the leaf hash in the Merkle tree
 * @param rootHash Root of the Merkle tree
 * @param proof A Merkle proof demonstrating membership of the leaf hash
 * @return True, if the leaf hash is in the Merkle tree; otherwise, False
*/
function checkMembership(bytes32 leaf, uint256 index, bytes32 rootHash, bytes memory proof)
    internal
    pure
    returns (bool)
{
    require(proof.length % 32 == 0, "Length of Merkle proof must be a multiple of 32");

    bytes32 proofElement;
    bytes32 computedHash = leaf;
    uint256 j = index;
    // Note: We're skipping the first 32 bytes of `proof`, which holds the size of the dynamically sized `bytes`
    for (uint256 i = 32; i <= proof.length; i += 32) {
        // solhint-disable-next-line no-inline-assembly
        assembly {
            proofElement := mload(add(proof, i))
        }
        if (j % 2 == 0) {
            computedHash = keccak256(abi.encodePacked(computedHash, proofElement));
        } else {
            computedHash = keccak256(abi.encodePacked(proofElement, computedHash));
        }
        j = j / 2;
    }

    return computedHash == rootHash;
}

The current implementation will validate the provided “leaf” and return true. This is a known problem of Merkle trees https://en.wikipedia.org/wiki/Merkle_tree#Second_preimage_attack.

Examples

Provide a hash from within the Merkle tree as the leaf argument. The index has to match the index of that node in regards to its current level in the tree. The rootHash has to be the correct Merkle tree rootHash. The proof has to skip the necessary number of levels because the nodes “underneath” the provided “leaf” will not be processed.

Recommendation

A remediation needs a fixed Merkle tree size as well as the addition of a byte prepended to each node in the tree. Another way would be to create a structure for the Merkle node and mark it as leaf or no leaf.

Corresponding issue in plasma-contracts repo: https://github.com/omisego/plasma-contracts/issues/425

Description

The plasma framework uses an ExitGameRegistry to allow the maintainer to add new exit games after deployment. An exit game is any arbitrary contract. In order to prevent the maintainer from adding malicious exit games that steal user funds, the framework uses a “quarantine” system whereby newly-registered exit games have restricted permissions until their quarantine period has expired. The quarantine period is by default 3 * minExitPeriod, and is intended to facilitate auditing of the new exit game’s functionality by the plasma users.

However, by registering an exit game at a contract which has not yet been deployed, the maintainer can prevent plasma users from auditing the game until the quarantine period has expired. After the quarantine period has expired, the maintainer can deploy the malicious exit game and immediately steal funds.

Explanation

Exit games are registered in the following function, callable only by the plasma contract maintainer:

code/plasma_framework/contracts/src/framework/registries/ExitGameRegistry.sol:L58-L78

/**
 * @notice Registers an exit game within the PlasmaFramework. Only the maintainer can call the function.
 * @dev Emits ExitGameRegistered event to notify clients
 * @param _txType The tx type where the exit game wants to register
 * @param _contract Address of the exit game contract
 * @param _protocol The transaction protocol, either 1 for MVP or 2 for MoreVP
 */
function registerExitGame(uint256 _txType, address _contract, uint8 _protocol) public onlyFrom(getMaintainer()) {
    require(_txType != 0, "Should not register with tx type 0");
    require(_contract != address(0), "Should not register with an empty exit game address");
    require(_exitGames[_txType] == address(0), "The tx type is already registered");
    require(_exitGameToTxType[_contract] == 0, "The exit game contract is already registered");
    require(Protocol.isValidProtocol(_protocol), "Invalid protocol value");

    _exitGames[_txType] = _contract;
    _exitGameToTxType[_contract] = _txType;
    _protocols[_txType] = _protocol;
    _exitGameQuarantine.quarantine(_contract);

    emit ExitGameRegistered(_txType, _contract, _protocol);
}

Notably, the function does not check the extcodesize of the submitted contract. As such, the maintainer can submit the address of a contract which does not yet exist and is not auditable.

After at least 3 * minExitPeriod seconds pass, the submitted contract now has full permissions as a registered exit game and can pass all checks using the onlyFromNonQuarantinedExitGame modifier:

code/plasma_framework/contracts/src/framework/registries/ExitGameRegistry.sol:L33-L40

/**
 * @notice A modifier to verify that the call is from a non-quarantined exit game
 */
modifier onlyFromNonQuarantinedExitGame() {
    require(_exitGameToTxType[msg.sender] != 0, "The call is not from a registered exit game contract");
    require(!_exitGameQuarantine.isQuarantined(msg.sender), "ExitGame is quarantined");
    _;
}

Additionally, the submitted contract passes checks made by external contracts using the isExitGameSafeToUse function:

code/plasma_framework/contracts/src/framework/registries/ExitGameRegistry.sol:L48-L56

/**
 * @notice Checks whether the contract is safe to use and is not under quarantine
 * @dev Exposes information about exit games quarantine
 * @param _contract Address of the exit game contract
 * @return boolean Whether the contract is safe to use and is not under quarantine
 */
function isExitGameSafeToUse(address _contract) public view returns (bool) {
    return _exitGameToTxType[_contract] != 0 && !_exitGameQuarantine.isQuarantined(_contract);
}

These permissions allow a registered quarantine to:

1. Withdraw any users’ tokens from ERC20Vault:

code/plasma_framework/contracts/src/vaults/Erc20Vault.sol:L52-L55

function withdraw(address payable receiver, address token, uint256 amount) external onlyFromNonQuarantinedExitGame {
    IERC20(token).safeTransfer(receiver, amount);
    emit Erc20Withdrawn(receiver, token, amount);
}

2. Withdraw any users’ ETH from EthVault:

code/plasma_framework/contracts/src/vaults/EthVault.sol:L46-L54

function withdraw(address payable receiver, uint256 amount) external onlyFromNonQuarantinedExitGame {
    // we do not want to block exit queue if transfer is unucessful
    // solhint-disable-next-line avoid-call-value
    (bool success, ) = receiver.call.value(amount)("");
    if (success) {
        emit EthWithdrawn(receiver, amount);
    } else {
        emit WithdrawFailed(receiver, amount);
    }

3. Activate and deactivate the ExitGameController reentrancy mutex:

code/plasma_framework/contracts/src/framework/ExitGameController.sol:L63-L66

function activateNonReentrant() external onlyFromNonQuarantinedExitGame() {
    require(!mutex, "Reentrant call");
    mutex = true;
}

code/plasma_framework/contracts/src/framework/ExitGameController.sol:L72-L75

function deactivateNonReentrant() external onlyFromNonQuarantinedExitGame() {
    require(mutex, "Not locked");
    mutex = false;
}

4. enqueue arbitrary exits:

code/plasma_framework/contracts/src/framework/ExitGameController.sol:L115-L138

function enqueue(
    uint256 vaultId,
    address token,
    uint64 exitableAt,
    TxPosLib.TxPos calldata txPos,
    uint160 exitId,
    IExitProcessor exitProcessor
)
    external
    onlyFromNonQuarantinedExitGame
    returns (uint256)
{
    bytes32 key = exitQueueKey(vaultId, token);
    require(hasExitQueue(key), "The queue for the (vaultId, token) pair is not yet added to the Plasma framework");
    PriorityQueue queue = exitsQueues[key];

    uint256 priority = ExitPriority.computePriority(exitableAt, txPos, exitId);

    queue.insert(priority);
    delegations[priority] = exitProcessor;

    emit ExitQueued(exitId, priority);
    return priority;
}

5. Flag outputs as “spent”:

code/plasma_framework/contracts/src/framework/ExitGameController.sol:L210-L213

function flagOutputSpent(bytes32 _outputId) external onlyFromNonQuarantinedExitGame {
    require(_outputId != bytes32(""), "Should not flag with empty outputId");
    isOutputSpent[_outputId] = true;
}

Recommendation

registerExitGame should check that extcodesize of the submitted contract is non-zero.

Corresponding issue in plasma-contracts repo: https://github.com/omisego/plasma-contracts/issues/410

Description

The state variable withdrawEntryCounter is not used in the code.

code/plasma_framework/contracts/src/vaults/EthVault.sol:L8

uint256 private withdrawEntryCounter = 0;

Recommendation

Remove it from the contract.

Description

Each plasma block has a maximum of 2 ** 16 transactions, which corresponds to a maximum Merkle tree height of 16. The Merkle library currently checks that the proof is comprised of 32-byte segments, but neglects to check the maximum height:

code/plasma_framework/contracts/src/utils/Merkle.sol:L17-L23

function checkMembership(bytes32 leaf, uint256 index, bytes32 rootHash, bytes memory proof)
    internal
    pure
    returns (bool)
{
    require(proof.length % 32 == 0, "Length of Merkle proof must be a multiple of 32");

Corresponding issue in plasma-contracts repo: https://github.com/omisego/plasma-contracts/issues/467

Description

The IsDeposit library is used to check whether a block number is a deposit or not. The logic is simple - if blockNum % childBlockInterval is nonzero, the block number is a deposit.

By including this check in BlockController instead, the contract can perform an existence check as well. The function in BlockController would return the same result as the IsDeposit library, but would additionally revert if the block in question does not exist:

function isDeposit(uint _blockNum) public view returns (bool) {
  require(blocks[_blockNum].timestamp != 0, "Block does not exist");
  return _blockNum % childBlockInterval != 0;
}

Note that this check is made at the cost of an external call. If the check needs to be made multiple times in a transaction, the result should be cached.

Corresponding issue in plasma-contracts repo: https://github.com/omisego/plasma-contracts/issues/466

Description

TxPosLib and UtxoPosLib serve very similar functions. They both provide utility functions to access the block number and tx index of a packed utxo position variable. UtxoPosLib, additionally, provides a function to retrieve the output index of a packed utxo position variable.

What they both lack, though, is sanity checks on the values packed inside a utxo position variable. By implementing a function UtxoPosLib.decode(uint _utxoPos) returns (UtxoPos), each exit controller contract can ensure that the values it is using make logical sense. The decode function should check that:

• txIndex is between 0 and 2**16
• outputIndex is between 0 and 3

Currently, neither of these restrictions is explicitly enforced. As for blockNum, the best check is that it exists in the PlasmaFramework contract with a nonzero root. Since UtxoPosLib is a pure library, that check is better performed elsewhere (See https://github.com/ConsenSys/omisego-morevp-audit-2019-10/issues/21).

Once implemented, all contracts should avoid casting values directly to the UtxoPos struct, in favor of using the decode function. Merging the two files will help with this.

Corresponding issue in plasma-contracts repo: https://github.com/omisego/plasma-contracts/issues/465

Description

Many input validation and storage read checks are made implicitly, rather than explicitly. The following compilation notes each line of code in the exit controller contracts where an additional check should be added.

Examples

1. PaymentChallengeIFEInputSpent:

• Check that inFlightTx has a nonzero input at the provided index:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEInputSpent.sol:L96

require(ife.isInputPiggybacked(args.inFlightTxInputIndex), "The indexed input has not been piggybacked");

• Check that each transaction is nonzero and is correctly formed:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEInputSpent.sol:L98-L101

require(
    keccak256(args.inFlightTx) != keccak256(args.challengingTx),
    "The challenging transaction is the same as the in-flight transaction"
);

• Check that resulting outputId is nonzero:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEInputSpent.sol:L123

bytes32 ifeInputOutputId = data.ife.inputs[data.args.inFlightTxInputIndex].outputId;

• See issue 5.10

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEInputSpent.sol:L125

UtxoPosLib.UtxoPos memory utxoPos = UtxoPosLib.UtxoPos(data.args.inputUtxoPos);

• See issue 5.9

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEInputSpent.sol:L126

bytes32 challengingTxInputOutputId = data.controller.isDeposit.test(utxoPos.blockNum())

• Check that inputTx is nonzero and well-formed:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEInputSpent.sol:L127-L128

? OutputId.computeDepositOutputId(data.args.inputTx, utxoPos.outputIndex(), utxoPos.value)
: OutputId.computeNormalOutputId(data.args.inputTx, utxoPos.outputIndex());

• Check that output is nonzero:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEInputSpent.sol:L149

WireTransaction.Output memory output = WireTransaction.getOutput(data.args.challengingTx, data.args.challengingTxInputIndex);

• See issue 5.10

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEInputSpent.sol:L156

UtxoPosLib.UtxoPos memory inputUtxoPos = UtxoPosLib.UtxoPos(data.args.inputUtxoPos);

• Check that challengingTx has a nonzero input at provided index:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEInputSpent.sol:L163

data.args.challengingTxInputIndex,

2. PaymentChallengeIFENotCanonical:

• Check that each transaction is nonzero and is correctly formed:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFENotCanonical.sol:L98-L101

require(
    keccak256(args.inFlightTx) != keccak256(args.competingTx),
    "The competitor transaction is the same as transaction in-flight"
);

• See issue 5.10

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFENotCanonical.sol:L104

UtxoPosLib.UtxoPos memory inputUtxoPos = UtxoPosLib.UtxoPos(args.inputUtxoPos);

• See issue 5.9

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFENotCanonical.sol:L107

if (self.isDeposit.test(inputUtxoPos.blockNum())) {

• Check that inputTx is nonzero and well-formed:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFENotCanonical.sol:L108-L110

    outputId = OutputId.computeDepositOutputId(args.inputTx, inputUtxoPos.outputIndex(), inputUtxoPos.value);
} else {
    outputId = OutputId.computeNormalOutputId(args.inputTx, inputUtxoPos.outputIndex());

• Check that inFlightTx has a nonzero input at the provided index:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFENotCanonical.sol:L112-L113

require(outputId == ife.inputs[args.inFlightTxInputIndex].outputId,
        "Provided inputs data does not point to the same outputId from the in-flight exit");

• Check that output is nonzero:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFENotCanonical.sol:L115

WireTransaction.Output memory output = WireTransaction.getOutput(args.inputTx, args.inFlightTxInputIndex);

• Check that competingTx has a nonzero input at provided index:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFENotCanonical.sol:L126

args.competingTxInputIndex,

• Check that resulting position is nonzero:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFENotCanonical.sol:L133

uint256 competitorPosition = verifyCompetingTxFinalized(self, args, output);

• Check that inFlightTxPos is nonzero:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFENotCanonical.sol:L171-L173

require(
    ife.oldestCompetitorPosition > inFlightTxPos,
    "In-flight transaction must be younger than competitors to respond to non-canonical challenge");

• See issue 5.10

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFENotCanonical.sol:L175

UtxoPosLib.UtxoPos memory utxoPos = UtxoPosLib.UtxoPos(inFlightTxPos);

• Check that block root is nonzero:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFENotCanonical.sol:L176

(bytes32 root, ) = self.framework.blocks(utxoPos.blockNum());

• Check that inFlightTx is nonzero and well-formed:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFENotCanonical.sol:L178

inFlightTx, utxoPos, root, inFlightTxInclusionProof

• See issue 5.10

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFENotCanonical.sol:L218

UtxoPosLib.UtxoPos memory competingTxUtxoPos = UtxoPosLib.UtxoPos(args.competingTxPos);

3. PaymentChallengeIFEOutputSpent:

• Check that inFlightTx is nonzero and is well-formed:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEOutputSpent.sol:L54

uint160 exitId = ExitId.getInFlightExitId(args.inFlightTx);

• See issue 5.10

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEOutputSpent.sol:L58

UtxoPosLib.UtxoPos memory utxoPos = UtxoPosLib.UtxoPos(args.outputUtxoPos);

• Check that inFlightTx has a nonzero output at the provided index:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEOutputSpent.sol:L60-L63

require(
    ife.isOutputPiggybacked(outputIndex),
    "Output is not piggybacked"
);

• Check that bond size is nonzero:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEOutputSpent.sol:L70

uint256 piggybackBondSize = ife.outputs[outputIndex].piggybackBondSize;

• See issue 5.10

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEOutputSpent.sol:L83

UtxoPosLib.UtxoPos memory utxoPos = UtxoPosLib.UtxoPos(args.outputUtxoPos);

• See issue 5.10

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEOutputSpent.sol:L101

UtxoPosLib.UtxoPos memory utxoPos = UtxoPosLib.UtxoPos(args.outputUtxoPos);

• Check that challengingTx is nonzero and is well-formed:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEOutputSpent.sol:L102

uint256 challengingTxType = WireTransaction.getTransactionType(args.challengingTx);

• Check that output is nonzero:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEOutputSpent.sol:L103

WireTransaction.Output memory output = WireTransaction.getOutput(args.challengingTx, utxoPos.outputIndex());

• Check that challengingTx has a nonzero input at provided index:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEOutputSpent.sol:L116

args.challengingTxInputIndex,

4. PaymentChallengeStandardExit:

• See issue 5.10

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeStandardExit.sol:L110

UtxoPosLib.UtxoPos memory utxoPos = UtxoPosLib.UtxoPos(data.exitData.utxoPos);

• Check that exitingTx is nonzero and well-formed:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeStandardExit.sol:L112

.decode(data.args.exitingTx)

• Check that output is nonzero:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeStandardExit.sol:L111-L113

PaymentOutputModel.Output memory output = PaymentTransactionModel
    .decode(data.args.exitingTx)
    .outputs[utxoPos.outputIndex()];

• Check that challengeTx is nonzero and well-formed:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeStandardExit.sol:L128

uint256 challengeTxType = WireTransaction.getTransactionType(data.args.challengeTx);

• See issue 5.10

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeStandardExit.sol:L134

txPos: TxPosLib.TxPos(data.args.challengeTxPos),

• See issue 5.9

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeStandardExit.sol:L157

bytes32 outputId = data.controller.isDeposit.test(utxoPos.blockNum())

• Check that challengeTx has a nonzero input at provided index:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeStandardExit.sol:L166

args.inputIndex,

5. PaymentPiggybackInFlightExit:

• Check that inFlightTx is nonzero and well-formed:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentPiggybackInFlightExit.sol:L93

uint160 exitId = ExitId.getInFlightExitId(args.inFlightTx);

• Check that inFlightTx has a nonzero input at provided index:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentPiggybackInFlightExit.sol:L99

require(!exit.isInputPiggybacked(args.inputIndex), "Indexed input already piggybacked");

• See issue 5.10

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentPiggybackInFlightExit.sol:L108

enqueue(self, withdrawData.token, UtxoPosLib.UtxoPos(exit.position), exitId);

• Check that inFlightTx is nonzero and is well-formed:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentPiggybackInFlightExit.sol:L130

uint160 exitId = ExitId.getInFlightExitId(args.inFlightTx);

• Check that inFlightTx has a nonzero output at provided index:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentPiggybackInFlightExit.sol:L136

require(!exit.isOutputPiggybacked(args.outputIndex), "Indexed output already piggybacked");

• See issue 5.10

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentPiggybackInFlightExit.sol:L147

enqueue(self, withdrawData.token, UtxoPosLib.UtxoPos(exit.position), exitId);

6. PaymentStartInFlightExit:

• Check that inFlightTx is nonzero and is well-formed:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentStartInFlightExit.sol:L146

exitData.exitId = ExitId.getInFlightExitId(args.inFlightTx);

• Check that the length of inputTxs is nonzero:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentStartInFlightExit.sol:L150

exitData.inputTxs = args.inputTxs;

• See issue 5.10

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentStartInFlightExit.sol:L167

utxosPos[i] = UtxoPosLib.UtxoPos(inputUtxosPos[i]);

• See issue 5.9

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentStartInFlightExit.sol:L180

bool isDepositTx = controller.isDeposit.test(utxoPos[i].blockNum());

• Check that each inputTxs is nonzero and well-formed:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentStartInFlightExit.sol:L181-L183

outputIds[i] = isDepositTx
    ? OutputId.computeDepositOutputId(inputTxs[i], utxoPos[i].outputIndex(), utxoPos[i].value)
    : OutputId.computeNormalOutputId(inputTxs[i], utxoPos[i].outputIndex());

• Check that each output is nonzero:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentStartInFlightExit.sol:L200

WireTransaction.Output memory output = WireTransaction.getOutput(inputTxs[i], outputIndex);

• Check that inFlightTx has nonzero inputs for all i:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentStartInFlightExit.sol:L327-L328

exitData.inFlightTxRaw,
i,

• Check that each output is nonzero:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentStartInFlightExit.sol:L407

PaymentOutputModel.Output memory output = exitData.inFlightTx.outputs[i];

7. PaymentStartStandardExit:

• See issue 5.10

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentStartStandardExit.sol:L119

UtxoPosLib.UtxoPos memory utxoPos = UtxoPosLib.UtxoPos(args.utxoPos);

• Check that output is nonzero:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentStartStandardExit.sol:L121

PaymentOutputModel.Output memory output = outputTx.outputs[utxoPos.outputIndex()];

• Check that timestamp is nonzero:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentStartStandardExit.sol:L124

(, uint256 blockTimestamp) = controller.framework.blocks(utxoPos.blockNum());

Corresponding issue in plasma-contracts repo: https://github.com/omisego/plasma-contracts/issues/463

Description

Several locations in the codebase feature unused arguments, functions, return values, and more. There are two primary reasons to remove these artifacts from the codebase:

1. Mass exits are the primary safeguard against a byzantine operator. The biggest bottleneck of a mass exit is transaction throughput, so plasma rootchain implementations should strive to be as efficient as possible. Many unused features require external calls, memory allocation, unneeded calculation, and more.

2. The contracts are set up to be extensible by way of the addition of new exit games to the system. “Optional” or unimplemented features in current exit games should be removed for simplicity’s sake, as they currently make up a large portion of the codebase.

Examples

• Output guard handlers

  • These offer very little utility in the current contracts. The main contract, PaymentOutputGuardHandler, has three functions:
    • isValid enforces that some “preimage” value passed in via calldata has a length of zero. This could be removed along with the unused “preimage” parameter.
    • getExitTarget converts a bytes20 to address payable (with the help of AddressPayable.sol). This could be removed in favor of using AddressPayable directly where needed.
    • getConfirmSigAddress simply returns an empty address. This should be removed wherever used - empty fields should be a rare exception or an error, rather than being injected as unused values into critical functions.
  • The minimal utility offered comes at the price of using an external call to the OutputGuardHandlerRegistry, as well as an external call for each of the functions mentioned above. Overall, the existence of output guard handlers adds thousands of gas to the exit process.
  • Referenced contracts: IOutputGuardHandler, OutputGuardModel, PaymentOutputGuardHandler, OutputGuardHandlerRegistry

• Payment router arguments

  • Several fields in the exit router structs are marked “optional,” and are not used in the contracts. While this is not particularly impactful, it does clutter and confuse the contracts. Many “optional” fields are referenced and passed into functions which do not use them. Of note is the crucially-important signature verification function, PaymentOutputToPaymentTxCondition.verify, where StartExitData.inputSpendingConditionOptionalArgs resolves to an unnamed parameter:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentStartInFlightExit.sol:L323-L332

bool isSpentByInFlightTx = condition.verify(
    exitData.inputTxs[i],
    exitData.inputUtxosPos[i].outputIndex(),
    exitData.inputUtxosPos[i].txPos().value,
    exitData.inFlightTxRaw,
    i,
    exitData.inFlightTxWitnesses[i],
    exitData.inputSpendingConditionOptionalArgs[i]
);
require(isSpentByInFlightTx, "Spending condition failed");

code/plasma_framework/contracts/src/exits/payment/spendingConditions/PaymentOutputToPaymentTxCondition.sol:L40-L47

function verify(
    bytes calldata inputTxBytes,
    uint16 outputIndex,
    uint256 inputTxPos,
    bytes calldata spendingTxBytes,
    uint16 inputIndex,
    bytes calldata signature,
    bytes calldata /*optionalArgs*/

The additional fields clutter the namespace of each struct, confusing the purpose of the other fields. For example, PaymentInFlightExitRouterArgs.StartExitArgs features two fields, inputTxsConfirmSigs and inFlightTxsWitnesses, the former of which is marked “optional”. In fact, the inFlightTxsWitnesses field ends up containing the signatures passed to the spending condition verifier and ECDSA library:

code/plasma_framework/contracts/src/exits/payment/routers/PaymentInFlightExitRouterArgs.sol:L4-L24

/**
* @notice Wraps arguments for startInFlightExit.
* @param inFlightTx RLP encoded in-flight transaction.
* @param inputTxs Transactions that created the inputs to the in-flight transaction. In the same order as in-flight transaction inputs.
* @param inputUtxosPos Utxos that represent in-flight transaction inputs. In the same order as input transactions.
* @param outputGuardPreimagesForInputs (Optional) Output guard pre-images for in-flight transaction inputs. Length must always match that of the inputTxs
* @param inputTxsInclusionProofs Merkle proofs that show the input-creating transactions are valid. In the same order as input transactions.
* @param inputTxsConfirmSigs (Optional) Confirm signatures for the input txs. Should be empty bytes if the input tx is MoreVP. Length must always match that of the inputTxs
* @param inFlightTxWitnesses Witnesses for in-flight transaction. In the same order as input transactions.
* @param inputSpendingConditionOptionalArgs (Optional) Additional args for the spending condition for checking inputs. Should provide empty bytes if nothing is required. Length must always match that of the inputTxs
*/
struct StartExitArgs {
    bytes inFlightTx;
    bytes[] inputTxs;
    uint256[] inputUtxosPos;
    bytes[] outputGuardPreimagesForInputs;
    bytes[] inputTxsInclusionProofs;
    bytes[] inputTxsConfirmSigs;
    bytes[] inFlightTxWitnesses;
    bytes[] inputSpendingConditionOptionalArgs;
}

Corresponding issue in plasma-contracts repo: https://github.com/omisego/plasma-contracts/issues/457

Description

RLP decoding is performed on transaction bytes in each of WireTransaction, PaymentOutputModel, and PaymentTransactionModel. The latter is the primary decoding function for transactions, while the former two contracts deal with outputs specifically.

Both WireTransaction and PaymentOutputModel make use of RLPReader to decode transaction objects, and both implement very similar features. Rather than having a codebase with two separate definitions for struct Output, PaymentTransactionModel should be extended to implement all required functionality.

Examples

• PaymentTransactionModel should include three distinct decoding functions:
  • decodeDepositTx decodes a deposit transaction, which has no inputs and exactly 1 output.
  • decodeSpendTx decodes a spend transaction, which has exactly 4 inputs and 4 outputs.
  • decodeOutput decodes an output, which is a long list with 4 fields (uint, address, address, uint)

A mock implementation including decodeSpendTx and decodeOutput is shown here: https://gist.github.com/wadeAlexC/7820c0cd82fd5fdc11a0ad58a84165ae

OmiseGo may want to consider enforcing restrictions on the ordering of empty and nonempty fields here as well.

Corresponding issue in plasma-contracts repo: https://github.com/omisego/plasma-contracts/issues/456

Description

The OpenZeppelin ECDSA library returns address(0x00) for many cases with malformed signatures:

contracts/cryptography/ECDSA.sol:L57-L63

if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) {
    return address(0);
}

if (v != 27 && v != 28) {
    return address(0);
}

The PaymentOutputToPaymentTxCondition contract does not explicitly handle this case:

code/plasma_framework/contracts/src/exits/payment/spendingConditions/PaymentOutputToPaymentTxCondition.sol:L65-L68

address payable owner = inputTx.outputs[outputIndex].owner();
require(owner == ECDSA.recover(eip712.hashTx(spendingTx), signature), "Tx in not signed correctly");

return true;

Recommendation

Adding a check to handle this case will make it easier to reason about the code.

Corresponding issue in plasma-contracts repo: https://github.com/omisego/plasma-contracts/issues/454

Description

The exit game libraries make several queries to the main PlasmaFramework contract where plasma block hashes and timestamps are stored. In multiple locations, the return values of these queries are not checked for existence.

Examples

1. PaymentStartStandardExit.setupStartStandardExitData:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentStartStandardExit.sol:L124

(, uint256 blockTimestamp) = controller.framework.blocks(utxoPos.blockNum());

2. PaymentChallengeIFENotCanonical.respond:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFENotCanonical.sol:L176

(bytes32 root, ) = self.framework.blocks(utxoPos.blockNum());

3. PaymentPiggybackInFlightExit.enqueue:

code/plasma_framework/contracts/src/exits/payment/controllers/PaymentPiggybackInFlightExit.sol:L167

(, uint256 blockTimestamp) = controller.framework.blocks(utxoPos.blockNum());

4. TxFinalizationVerifier.checkInclusionProof:

code/plasma_framework/contracts/src/exits/utils/TxFinalizationVerifier.sol:L54

(bytes32 root,) = data.framework.blocks(data.txPos.blockNum());

Recommendation

Although none of these examples seem exploitable, adding existence checks makes it easier to reason about the code. Each query to PlasmaFramework.blocks should be followed with a check that the returned value is nonzero.

Corresponding issue in plasma-contracts repo: https://github.com/omisego/plasma-contracts/issues/463

Description

In BondSize, the mechanism to update the size of the bond has a grace period after which the new bond size becomes active.

When updating the bond size, the time is casted as a uint64 and saved in a uint128 variable.

code/plasma_framework/contracts/src/exits/utils/BondSize.sol:L24

uint128 effectiveUpdateTime;

code/plasma_framework/contracts/src/exits/utils/BondSize.sol:L11

uint64 constant public WAITING_PERIOD = 2 days;

code/plasma_framework/contracts/src/exits/utils/BondSize.sol:L57

self.effectiveUpdateTime = uint64(now) + WAITING_PERIOD;

There’s no need to use a uint128 to save the time if it never will take up that much space.

Recommendation

Change the type of the effectiveUpdateTime to uint64.

- uint128 effectiveUpdateTime;
+ uint64 effectiveUpdateTime;

Description

PaymentExitGame inherits from both PaymentInFlightExitRouter and PaymentStandardExitRouter. All three contracts declare and initialize their own PlasmaFramework variable. This pattern can be misleading, and may lead to subtle issues in future versions of the code.

Examples

1. PaymentExitGame declaration:

code/plasma_framework/contracts/src/exits/payment/PaymentExitGame.sol:L18

PlasmaFramework private plasmaFramework;

2. PaymentInFlightExitRouter declaration:

code/plasma_framework/contracts/src/exits/payment/routers/PaymentInFlightExitRouter.sol:L53

PlasmaFramework private framework;

3. PaymentStandardExitRouter declaration:

code/plasma_framework/contracts/src/exits/payment/routers/PaymentStandardExitRouter.sol:L45

PlasmaFramework private framework;

Each variable is initialized in the corresponding file’s constructor.

Recommendation

Introduce an inherited contract common to PaymentStandardExitRouter and PaymentInFlightExitRouter with the PlasmaFramework variable. Make the variable internal so it is visible to inheriting contracts.

Description

The Vault calls submitDepositBlock when a user deposits funds into the plasma chain. Each deposit transaction creates one deposit block on the plasma chain. The number of deposit blocks between two child blocks is limited by the childBlockInterval. For example, a childBlockInterval of 1 would not allow any deposit blocks, a childBlockInterval of 2 would allow one deposit block after each child block [child][optional: deposit][child][optional: deposit].

code/plasma_framework/contracts/src/framework/BlockController.sol:L96-L114

/**
 * @notice Submits a block for deposit
 * @dev Block number adds 1 per submission; it's possible to have at most 'childBlockInterval' deposit blocks between two child chain blocks
 * @param _blockRoot Merkle root of the Plasma block
 * @return The deposit block number
 */
function submitDepositBlock(bytes32 _blockRoot) public onlyFromNonQuarantinedVault returns (uint256) {
    require(isChildChainActivated == true, "Child chain has not been activated by authority address yet");
    require(nextDeposit < childBlockInterval, "Exceeded limit of deposits per child block interval");

    uint256 blknum = nextDepositBlock();
    blocks[blknum] = BlockModel.Block({
        root : _blockRoot,
        timestamp : block.timestamp
    });

    nextDeposit++;
    return blknum;
}

However, the comment at line 98 mentions the following:

[..] it’s possible to have at most ‘childBlockInterval’ deposit blocks between two child chain blocks [..]

This comment is inaccurate, as a childBlockInterval of 1 would not allow deposits at all (Note how nextDeposit is always >=1).

Remediation

The comment should read: [..] it’s possible to have at most ‘childBlockInterval -1’ deposit blocks between two child chain blocks [..]. Make sure to properly validate inputs for these values when deploying the contract to avoid obvious misconfiguration.

Description

The contract PlasmaFramework inherits VaultRegistry even though it does not use any of the methods directly. Also BlockController inherits VaultRegistry effectively adding all of the needed functionality in there.

Remediation

PlasmaFramework does not need to inherit VaultRegistry, thus the import and the inheritance can be removed from PlasmaFramework.sol.

 import "./BlockController.sol";
 import "./ExitGameController.sol";
-import "./registries/VaultRegistry.sol";
 import "./registries/ExitGameRegistry.sol";

-contract PlasmaFramework is VaultRegistry, ExitGameRegistry, ExitGameController, BlockController {
+contract PlasmaFramework is ExitGameRegistry, ExitGameController, BlockController {
     uint256 public constant CHILD_BLOCK_INTERVAL = 1000;

     /**

All tests still pass after removing the inheritance.

Description

code/plasma_framework/contracts/src/framework/BlockController.sol:L69-L72

function setAuthority(address newAuthority) external onlyFrom(authority) {
    require(newAuthority != address(0), "Authority address cannot be zero");
    authority = newAuthority;
}

deployer initially sets the account that is allowed to submit new blocks as authority. authority can then set a new authority at will. In a system that is set-up and maintained by a maintainer role (multi-sig) that can upgrade certain parts of the system it is unexpected for another role to be able to pass along its permissions. The security specification notes that the authority role is only used to submit blocks:

Authority: EOA used exclusively to submit plasma block hashes to the root chain. The child chain assumes at deployment that the authority account has nonce zero and no transactions have been sent from it.

However, no transactions might not be possible as authority is the only one to activateChildChain. Once activated, the child chain cannot be de-activated but the authority can change.

elixir-omg#managing-the-operator-address notes the following for operator aka authority:

As a consequence, the operator address must never send any other transactions, if it intends to continue submitting blocks. (Workarounds to this limitation are available, if there’s such requirement.)

Additionally, setAuthority should emit an event to allow participants to react to this change in the system and have an audit trial.

Remediation

Remove the setAuthority function, or clarify its intended purpose and add an event so it can be detected by users.

Corresponding issue in plasma-contracts repo: https://github.com/omisego/plasma-contracts/issues/403