Security Review Limit Order Contracts

NOTES.md

@@ -0,0 +1 @@
+- On old LimitOrder / SignedOperationProxy should disable operation and set owner to 0 before going live

__tests__/proxies/SignedOperationProxy.test.ts

@@ -995,6 +995,9 @@ describe('SignedOperationProxy', () => {
       ]);
       await expectInvalid([signedWithdrawOperation, signedOperation2]);
     });
+
+    // Maybe also test:
+    // - authorization that overflows the actions arrays
   });
 });
 

contracts/external/lib/TypedSignature.sol

@@ -45,6 +45,7 @@ library TypedSignature {
 
     // ============ Enums ============
 
+    // Put some commets as to what these types are
     enum SignatureType {
         NoPrepend,
         Decimal,
@@ -82,6 +83,7 @@ library TypedSignature {
         uint8 rawSigType;
 
         /* solium-disable-next-line security/no-inline-assembly */
+        // Can we use abi.decode here?
         assembly {
             r := mload(add(signatureWithType, 0x20))
             s := mload(add(signatureWithType, 0x40))

contracts/external/proxies/SignedOperationProxy.sol

@@ -77,9 +77,9 @@ contract SignedOperationProxy is
     // EIP712 encodeType of Action
     bytes constant private EIP712_ACTION_STRING = abi.encodePacked(
         "Action(",
-        "uint8 actionType,",
+        "uint8 actionType,", // Is it ok that this is uint8?
         "address accountOwner,",
-        "uint256 accountNumber,",
+        "uint256 accountNumber,", // We call this accountId elsewhere
         "AssetAmount assetAmount,",
         "uint256 primaryMarketId,",
         "uint256 secondaryMarketId,",
@@ -233,6 +233,7 @@ contract SignedOperationProxy is
     )
         public
     {
+        // Don't think it matters functionally, but maybe also require auth.numActions == actions.length
         bytes32 operationHash = getOperationHash(
             accounts,
             actions,
@@ -296,10 +297,13 @@ contract SignedOperationProxy is
             );
 
             // consider the signer to be msg.sender unless there is a signature
+            // This is weird because validateAccountOwner already special cases msg.sender
+            // Probably just set it to 0
             address signer = msg.sender;
 
             // if there is a signature, then validate it
             if (auth.signature.length != 0) {
+                // Split this out into a helper function
                 // get the hash of the operation
                 bytes32 operationHash = getOperationHash(
                     accounts,
@@ -333,6 +337,8 @@ contract SignedOperationProxy is
 
             // cache the index of the first action after this auth
             uint256 actionEndIdx = actionStartIdx.add(auth.numActions);
+            // may be better to explicitly require actionEndIdx < actions.length
+            // rather than relying on solidity out of bounds checks
 
             // loop over all actions for which this auth applies
             for (uint256 actionIdx = actionStartIdx; actionIdx < actionEndIdx; actionIdx++) {
@@ -397,6 +403,9 @@ contract SignedOperationProxy is
         view
     {
         bool valid =
+         // THOUGHT: Wouldn't signer now be set to msg.sender if no signature, so this is unneeded?
+         // answer: this is still needed because you could have an action block that touches
+         // both signer and msg.sender accounts
             msg.sender == accountOwner
             || signer == accountOwner
             || SOLO_MARGIN.getIsLocalOperator(accountOwner, msg.sender)
@@ -466,9 +475,11 @@ contract SignedOperationProxy is
 
         // for each action that corresponds to the auth
         for (uint256 i = 0; i < auth.numActions; i++) {
+            // what happens if auth.numActions > actions.length? Maybe we should just require this
             Actions.ActionArgs memory action = actions[startIdx + i];
 
             // if action type has no second account, assume null account
+            // what happens if action.otherAccountId > accounts.length? Maybe we should just require this
             Account.Info memory otherAccount =
                 (Actions.getAccountLayout(action.actionType) == Actions.AccountLayout.OnePrimary)
                 ? Account.Info({ owner: address(0), number: 0 })

contracts/external/traders/LimitOrders.sol

@@ -461,6 +461,7 @@ contract LimitOrders is
 
         // verify taker
         Require.that(
+            // Line over max-len, does linter not catch this?
             (orderInfo.order.takerAccountOwner == address(0) && orderInfo.order.takerAccountNumber == 0 ) ||
             (orderInfo.order.takerAccountOwner == takerAccount.owner && orderInfo.order.takerAccountNumber == takerAccount.number),
             FILE,
@@ -564,6 +565,7 @@ contract LimitOrders is
         return totalMakerFilledAmount;
     }
 
+    // Nit: order these helper functions in the order they are used
     /**
      * Parses the order, verifies that it is not expired or canceled, and verifies the signature.
      */
@@ -575,6 +577,7 @@ contract LimitOrders is
         returns (OrderInfo memory)
     {
         Require.that(
+          // Should validate it is either exactly NUM_ORDER_BYTES or NUM_ORDER_BYTES + NUM_SIGNATURE_BYTES
             data.length >= NUM_ORDER_BYTES,
             FILE,
             "Cannot parse order from data"
@@ -625,6 +628,7 @@ contract LimitOrders is
         /* solium-disable-next-line indentation */
         bytes32 structHash = keccak256(abi.encode(
             EIP712_LIMIT_ORDER_STRUCT_SCHEMA_HASH,
+            // Does abi encoding structs just lay out all fields side by side with no padding?
             order
         ));
 
@@ -648,6 +652,7 @@ contract LimitOrders is
         returns (bytes memory)
     {
         Require.that(
+            // Why not exactly NUM_ORDER_BYTES + NUM_SIGNATURE_BYTES?
             data.length >= NUM_ORDER_BYTES + NUM_SIGNATURE_BYTES,
             FILE,
             "Cannot parse signature from data"

contracts/protocol/interfaces/ICallee.sol

@@ -40,7 +40,9 @@ contract ICallee {
      * @param  data         Arbitrary data given by the sender
      */
     function callFunction(
-        address sender,
+        address sender, // This is set to msg.sender, so this would be the auth proxy for everything...
+        // None of our contracts even use this right now though
+        // Same could be said of the payable proxy
         Account.Info memory accountInfo,
         bytes memory data
     )