## General

* A lot of code is unused / boilerplate for the extensibility features. The amount of file hopping this creates makes it really hard to review.
* Especially with Plasma -- "don't code it 'till you need it"
* Any extensibility mechanism in the code may break security properties, and allowing for that extensibility before it is needed obfuscates current security properties
* Examples:
* `PaymentChallengeIFEInputSpent.verifyChallengingTransactionProtocolFinalized` references 4+ structs and 3+ files just to check that `challengingTx.length != 0`
* SpendingConditionRegistry / spendingConditions
* OutputGuardHandlerRegistry / PaymentOutputGuardHandler doesn't provide any utility, but makes up a lot of code:
* Necessitates unused parameters like preimages
* `data.outputGuardHandler.getExitTarget(data.outputGuardData)` references several structs and files and results in simply casting a `bytes20` to an `address`
* `getConfirmSigAddress` feeds an empty address into a critical verification function and it isn't even used.
* Plus, the actual signature that gets checked is in a struct field referred to as a "witness", not a "signature"

* There seems to be a lack of existence and bounds checks for a lot of inputs. An entire network is represented on-chain by the contract's interpretation of user inputs. It's critical that the contract:
* Is _dead simple_
* Is _very strict_
* Validates that _absolutely every_ input, output, calculation, and storage read falls within expected / reasonable ranges
* Examples:
* No existence check on plasma block and timestamp reads
* `PaymentChallengeIFENotCanonical.respond` blindly accepts almost any value for `inFlightTxPos`

* A lot of security relies on the migration and initialization process.
* Knowing what values were allowed for `supportInputTxType` and `supportSpendingType` in `PaymentOutputToPaymentTxCondition` depended on reviewing the migration script as well as a config file.
* It's easy to validate that code was deployed correctly, but hard to validate that code is initialized correctly. The more initialization is performed in the constructor, the easier it is to understand what the system looks like.

* Hypothetical - a user has deployed a Watcher and the Watcher picks up an issue. What happens next?
* Is there a mass exit? How is this coordinated? Do users have some sort of frontend through which
they can generate the transaction for their mass exit?

* Fees are not implemented, but the contracts contain logic for them.

## Specific

* Sometimes exitId is calculated, and sometimes it is passed in as an argument in calldata
* Given the variable length encoding of input transactions, it concerns me that a passed-in encoded transaction and exit id feel so loosely related.
* (See ChallengeStandardExitArgs vs any other struct)

* The ECDSA library being used returns address(0x00) on error. Every call to this function should handle this case.
* (See PaymentOutputToPaymentTxCondition.verify)

* Why is there a separate outputId and exitId?
* Both use utxoPos.outputIndex, so both should uniquely represent a single output

* `PaymentChallengeIFENotCanonical.respond` allows a user to initiate a response, even if a challenge has not been initiated.

* `utxoPos` values are often unchecked and may fall well outside of expected ranges. (See `PaymentChallengeIFENotCanonical.respond`)

* `clearOutputPiggyback` vs `clearOutputPiggybacked` in `PaymentInFlightExitModelUtils`

### PaymentStartInFlightExit

* There is only 1 check that the input transactions provided are nonzero: line 1 of `getYoungetInputUtxoPosition` is an array access that will revert if length is zero.
* If the person who wrote this made the reasonable decision to start `uint youngest` at 0 and iterate over the whole array, anyone would be able to start an un-challengable IFE with 0 inputs (or possibly IFE deposit txns)

### PaymentOutputToPaymentTxCondition

* `verify` params `outputIndex` and `inputTxPos` seem to have little purpose. Could just pass the utxoPos and calculate values as needed
* This is done anyway when called - (See PaymentChallengeStandardExit and PaymentStartInFlightExit)

## Daniel's notes

### Different implementation patterns for the same kind of functionality

* method `BondSize.updateBondSize` updates the bond size in this manner:

    if (self.updatedBondSize != 0 && now >= self.effectiveUpdateTime) {
        self.previousBondSize = self.updatedBondSize;
    }
    self.updatedBondSize = newBondSize;
    self.effectiveUpdateTime = uint64(now) + WAITING_PERIOD;

* method `Vault.setDepositVerifier` updates a deposit verifier in this manner:

if (depositVerifiers[0] != address(0)) {
    depositVerifiers[0] = getEffectiveDepositVerifier();
    depositVerifiers[1] = _verifier;
    newDepositVerifierMaturityTimestamp = now + framework.minExitPeriod();
} else {
    depositVerifiers[0] = _verifier;
}

emit SetDepositVerifierCalled(_verifier);

The 2 pieces of code behave the same, but they are written very differently, the same pattern (any of the 2) can be applied in both places achieving the same functionality.

Also the event is emitted in different parts of the call graph.

### Some contracts do not use all imports

- Example `PaymentChallengeIFEInputSpent.sol` imports `Merkle.sol`, `OutputGuardModel.sol`, `IOutputGuardHandler.sol` but never uses them.

There can be multiple instances where this happens.

### The setup is important for the app's correctness

`PaymentStandardExitRouter.constructor` could have `ethVaultId` and `erc20VaultId` mixed up, there's no additional check in the constructor about the correctness of each vault.

address ethVaultAddress = plasmaFramework.vaults(ethVaultId);
require(ethVaultAddress != address(0), "Invalid ETH vault");
EthVault ethVault = EthVault(ethVaultAddress);

address erc20VaultAddress = plasmaFramework.vaults(erc20VaultId);
require(erc20VaultAddress != address(0), "Invalid ERC20 vault");
Erc20Vault erc20Vault = Erc20Vault(erc20VaultAddress);

### Safe math is not used everywhere

In `BondSize` safe math operations are not enforced.

require(newBondSize >= currentBondSize / self.lowerBoundDivisor, "Bond size is too low");
require(uint256(newBondSize) <= uint256(currentBondSize) * self.upperBoundMultiplier, "Bond size is too high");