[M01] limit trustedIssuers in Escrow payments

packages/protocol/contracts/identity/Escrow.sol

@@ -87,6 +87,10 @@ contract Escrow is
   // Governable list of trustedIssuers to set for payments by default.
   address[] public defaultTrustedIssuers;
 
+  // Based on benchmarking of FederatedAttestations lookup gas consumption
+  // in the worst case (with a significant amount of buffer).
+  uint256 public constant MAX_TRUSTED_ISSUERS_PER_PAYMENT = 100;
+
   /**
    * @notice Sets initialized == true on implementation contracts
    * @param test Set to true to skip implementation initialization
@@ -112,9 +116,15 @@ contract Escrow is
    * @notice Add an address to the defaultTrustedIssuers list.
    * @param trustedIssuer Address of the trustedIssuer to add.
    * @dev Throws if trustedIssuer is null or already in defaultTrustedIssuers.
+   * @dev Throws if defaultTrustedIssuers is already at max allowed length.
    */
   function addDefaultTrustedIssuer(address trustedIssuer) external onlyOwner {
     require(address(0) != trustedIssuer, "trustedIssuer can't be null");
+    require(
+      defaultTrustedIssuers.length.add(1) <= MAX_TRUSTED_ISSUERS_PER_PAYMENT,
+      "defaultTrustedIssuers.length can't exceed allowed number of trustedIssuers"
+    );
+
     // Ensure list of trusted issuers is unique
     for (uint256 i = 0; i < defaultTrustedIssuers.length; i = i.add(1)) {
       require(
@@ -485,6 +495,13 @@ contract Escrow is
       "trustedIssuers may only be set when attestations are required"
     );
 
+    // Ensure that withdrawal will not fail due to exceeding trustedIssuer limit
+    // in FederatedAttestations.lookupAttestations
+    require(
+      trustedIssuers.length <= MAX_TRUSTED_ISSUERS_PER_PAYMENT,
+      "Too many trustedIssuers provided"
+    );
+
     IAttestations attestations = getAttestations();
     require(
       minAttestations <= attestations.getMaxAttestations(),

packages/protocol/contracts/identity/FederatedAttestations.sol

@@ -55,6 +55,9 @@ contract FederatedAttestations is
     )
   );
 
+  // Changing any of these constraints will require re-benchmarking
+  // and checking assumptions for batch revocation.
+  // These can only be modified by releasing a new version of this contract.
   uint256 public constant MAX_ATTESTATIONS_PER_IDENTIFIER = 20;
   uint256 public constant MAX_IDENTIFIERS_PER_ADDRESS = 20;
 

packages/protocol/contracts/identity/interfaces/IEscrow.sol

@@ -26,6 +26,7 @@ interface IEscrow {
   function getSentPaymentIds(address sender) external view returns (address[] memory);
   function getTrustedIssuersPerPayment(address paymentId) external view returns (address[] memory);
   function getDefaultTrustedIssuers() external view returns (address[] memory);
+  function MAX_TRUSTED_ISSUERS_PER_PAYMENT() external view returns (uint256);
 
   // onlyOwner functions
   function addDefaultTrustedIssuer(address trustedIssuer) external;

packages/protocol/test/identity/escrow.ts

@@ -162,10 +162,39 @@ contract('Escrow', (accounts: string[]) => {
       )
     })
 
-    it('should allow a second trustedIssuer to be added', async () => {
-      await escrow.addDefaultTrustedIssuer(trustedIssuer1, { from: owner })
-      await escrow.addDefaultTrustedIssuer(trustedIssuer2, { from: owner })
-      assert.deepEqual(await escrow.getDefaultTrustedIssuers(), [trustedIssuer1, trustedIssuer2])
+    describe('when max trusted issuers have been added', async () => {
+      let expectedTrustedIssuers: string[]
+
+      beforeEach(async () => {
+        const maxTrustedIssuers = (await escrow.MAX_TRUSTED_ISSUERS_PER_PAYMENT()).toNumber()
+        expectedTrustedIssuers = []
+        for (let i = 0; i < maxTrustedIssuers; i++) {
+          const newIssuer = await web3.eth.accounts.create().address
+          await escrow.addDefaultTrustedIssuer(newIssuer, { from: owner })
+          expectedTrustedIssuers.push(newIssuer)
+        }
+      })
+
+      it('should have set expected default trusted issuers', async () => {
+        assert.deepEqual(await escrow.getDefaultTrustedIssuers(), expectedTrustedIssuers)
+      })
+
+      it('should not allow more trusted issuers to be added', async () => {
+        await assertRevertWithReason(
+          escrow.addDefaultTrustedIssuer(trustedIssuer1, { from: owner }),
+          "defaultTrustedIssuers.length can't exceed allowed number of trustedIssuers"
+        )
+      })
+
+      it('should allow removing and adding an issuer', async () => {
+        await escrow.removeDefaultTrustedIssuer(expectedTrustedIssuers[0], 0, { from: owner })
+        await escrow.addDefaultTrustedIssuer(trustedIssuer1)
+        expectedTrustedIssuers.push(trustedIssuer1)
+        assert.deepEqual(
+          (await escrow.getDefaultTrustedIssuers()).sort(),
+          expectedTrustedIssuers.slice(1).sort()
+        )
+      })
     })
   })
 
@@ -370,6 +399,23 @@ contract('Escrow', (accounts: string[]) => {
         )
       })
 
+      it('should allow transfer when max trustedIssuers are provided', async () => {
+        const repeatedTrustedIssuers = Array.from<string>({
+          length: (await escrow.MAX_TRUSTED_ISSUERS_PER_PAYMENT()).toNumber(),
+        }).fill(trustedIssuer1)
+        await transferAndCheckState(
+          sender,
+          aPhoneHash,
+          aValue,
+          oneDayInSecs,
+          withdrawKeyAddress,
+          3,
+          repeatedTrustedIssuers,
+          [withdrawKeyAddress],
+          [withdrawKeyAddress]
+        )
+      })
+
       it('should allow transfer when no identifier is provided', async () => {
         await transferAndCheckState(
           sender,
@@ -485,6 +531,25 @@ contract('Escrow', (accounts: string[]) => {
         )
       })
 
+      it('should not allow a transfer when too many trustedIssuers are provided', async () => {
+        const repeatedTrustedIssuers = Array.from<string>({
+          length: (await escrow.MAX_TRUSTED_ISSUERS_PER_PAYMENT()).toNumber() + 1,
+        }).fill(trustedIssuer1)
+        await assertRevertWithReason(
+          escrow.transferWithTrustedIssuers(
+            aPhoneHash,
+            mockERC20Token.address,
+            aValue,
+            oneDayInSecs,
+            withdrawKeyAddress,
+            3,
+            repeatedTrustedIssuers,
+            { from: sender }
+          ),
+          'Too many trustedIssuers provided'
+        )
+      })
+
       it('should not allow a transfer if token is 0', async () => {
         await assertRevert(
           escrow.transferWithTrustedIssuers(