[M05] Missing counter addition

packages/protocol/contracts/identity/Attestations.sol

@@ -82,6 +82,9 @@ contract Attestations is
   // The duration to wait until selectIssuers can be called for an attestation request.
   uint256 public selectIssuersWaitBlocks;
 
+  // Limit the maximum number of attestations that can be requested
+  uint256 public maxAttestations;
+
   // The fees that are associated with attestations for a particular token.
   mapping(address => uint256) public attestationRequestFees;
 
@@ -112,6 +115,7 @@ contract Attestations is
   event AttestationExpiryBlocksSet(uint256 value);
   event AttestationRequestFeeSet(address indexed token, uint256 value);
   event SelectIssuersWaitBlocksSet(uint256 value);
+  event MaxAttestationsSet(uint256 value);
 
   /**
    * @notice Used in place of the constructor to allow the contract to be upgradable via proxy.
@@ -127,13 +131,15 @@ contract Attestations is
     address registryAddress,
     uint256 _attestationExpiryBlocks,
     uint256 _selectIssuersWaitBlocks,
+    uint256 _maxAttestations,
     address[] calldata attestationRequestFeeTokens,
     uint256[] calldata attestationRequestFeeValues
   ) external initializer {
     _transferOwnership(msg.sender);
     setRegistry(registryAddress);
     setAttestationExpiryBlocks(_attestationExpiryBlocks);
     setSelectIssuersWaitBlocks(_selectIssuersWaitBlocks);
+    setMaxAttestations(_maxAttestations);
 
     require(
       attestationRequestFeeTokens.length > 0 &&
@@ -171,6 +177,7 @@ contract Attestations is
     );
 
     require(attestationsRequested > 0, "You have to request at least 1 attestation");
+    require(attestationsRequested <= maxAttestations, "Too many attestations requested");
 
     IdentifierState storage state = identifiers[identifier];
 
@@ -497,6 +504,24 @@ contract Attestations is
     emit SelectIssuersWaitBlocksSet(_selectIssuersWaitBlocks);
   }
 
+  /**
+   * @notice Updates 'maxAttestations'.
+   * @param _maxAttestations Maximum number of attestations that can be requested.
+   */
+  function setMaxAttestations(uint256 _maxAttestations) public onlyOwner {
+    require(_maxAttestations > 0, "maxAttestations has to be greater than 0");
+    maxAttestations = _maxAttestations;
+    emit MaxAttestationsSet(_maxAttestations);
+  }
+
+  /**
+   * @notice Query 'maxAttestations'
+   * @return Maximum number of attestations that can be requested.
+   */
+  function getMaxAttestations() external view returns (uint256) {
+    return maxAttestations;
+  }
+
   /**
    * @notice Validates the given attestation code.
    * @param identifier The hash of the identifier to be attested.
@@ -576,41 +601,48 @@ contract Attestations is
     bytes32 seed = getRandom().getBlockRandomness(
       uint256(unselectedRequest.blockNumber).add(selectIssuersWaitBlocks)
     );
-    uint256 numberValidators = numberValidatorsInCurrentSet();
+    IAccounts accounts = getAccounts();
+    uint256 issuersLength = numberValidatorsInCurrentSet();
+    uint256[] memory issuers = new uint256[](issuersLength);
+    for (uint256 i = 0; i < issuersLength; i++) issuers[i] = i;
+
+    require(unselectedRequest.attestationsRequested <= issuersLength, "not enough issuers");
 
     uint256 currentIndex = 0;
-    address validator;
-    address issuer;
-    IAccounts accounts = getAccounts();
 
+    // The length of the list (variable issuersLength) is decremented in each round,
+    // so the loop always terminates
     while (currentIndex < unselectedRequest.attestationsRequested) {
+      require(issuersLength > 0, "not enough issuers");
       seed = keccak256(abi.encodePacked(seed));
-      validator = validatorSignerAddressFromCurrentSet(uint256(seed) % numberValidators);
-      issuer = accounts.signerToAccount(validator);
-
-      if (!accounts.hasAuthorizedAttestationSigner(issuer)) {
-        continue;
-      }
+      uint256 idx = uint256(seed) % issuersLength;
+      address signer = validatorSignerAddressFromCurrentSet(issuers[idx]);
+      address issuer = accounts.signerToAccount(signer);
 
       Attestation storage attestation = state.issuedAttestations[issuer];
 
-      // Attestation issuers can only be added if they haven't been already.
-      if (attestation.status != AttestationStatus.None) {
-        continue;
+      if (
+        attestation.status == AttestationStatus.None &&
+        accounts.hasAuthorizedAttestationSigner(issuer)
+      ) {
+        currentIndex = currentIndex.add(1);
+        attestation.status = AttestationStatus.Incomplete;
+        attestation.blockNumber = unselectedRequest.blockNumber;
+        attestation.attestationRequestFeeToken = unselectedRequest.attestationRequestFeeToken;
+        state.selectedIssuers.push(issuer);
+
+        emit AttestationIssuerSelected(
+          identifier,
+          msg.sender,
+          issuer,
+          unselectedRequest.attestationRequestFeeToken
+        );
       }
 
-      currentIndex = currentIndex.add(1);
-      attestation.status = AttestationStatus.Incomplete;
-      attestation.blockNumber = unselectedRequest.blockNumber;
-      attestation.attestationRequestFeeToken = unselectedRequest.attestationRequestFeeToken;
-      state.selectedIssuers.push(issuer);
-
-      emit AttestationIssuerSelected(
-        identifier,
-        msg.sender,
-        issuer,
-        unselectedRequest.attestationRequestFeeToken
-      );
+      // Remove the validator that was selected from the list,
+      // by replacing it by the last element in the list
+      issuersLength = issuersLength.sub(1);
+      issuers[idx] = issuers[issuersLength];
     }
   }
 

packages/protocol/contracts/identity/Escrow.sol

@@ -99,6 +99,12 @@ contract Escrow is IEscrow, ReentrancyGuard, Ownable, Initializable, UsingRegist
       "Invalid privacy inputs: Can't require attestations if no identifier"
     );
 
+    IAttestations attestations = IAttestations(registry.getAddressFor(ATTESTATIONS_REGISTRY_ID));
+    require(
+      minAttestations <= attestations.getMaxAttestations(),
+      "minAttestations larger than limit"
+    );
+
     uint256 sentIndex = sentPaymentIds[msg.sender].push(paymentId).sub(1);
     uint256 receivedIndex = receivedPaymentIds[identifier].push(paymentId).sub(1);
 

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

@@ -1,8 +1,6 @@
 pragma solidity ^0.5.3;
 
 interface IAttestations {
-  function initialize(address, uint256, uint256, address[] calldata, uint256[] calldata) external;
-
   function setAttestationRequestFee(address, uint256) external;
   function request(bytes32, uint256, address) external;
   function selectIssuers(bytes32) external;
@@ -12,6 +10,8 @@ interface IAttestations {
 
   function setAttestationExpiryBlocks(uint256) external;
 
+  function getMaxAttestations() external view returns (uint256);
+
   function getUnselectedRequest(bytes32, address) external view returns (uint32, uint32, address);
   function getAttestationRequestFee(address) external view returns (uint256);
 

packages/protocol/contracts/identity/test/MockAttestations.sol

@@ -30,6 +30,10 @@ contract MockAttestations {
     identifiers[identifier].attestations[msg.sender].completed++;
   }
 
+  function getMaxAttestations() external pure returns (uint256) {
+    return 20;
+  }
+
   function getAttestationStats(bytes32 identifier, address account)
     external
     view

packages/protocol/migrations/17_attestations.ts

@@ -6,7 +6,7 @@ import {
 } from '@celo/protocol/lib/web3-utils'
 import { config } from '@celo/protocol/migrationsConfig'
 import { AttestationsInstance, StableTokenInstance } from 'types'
-const initializeArgs = async (): Promise<[string, string, string, string[], string[]]> => {
+const initializeArgs = async (): Promise<[string, string, string, string, string[], string[]]> => {
   const stableToken: StableTokenInstance = await getDeployedProxiedContract<StableTokenInstance>(
     'StableToken',
     artifacts
@@ -20,6 +20,7 @@ const initializeArgs = async (): Promise<[string, string, string, string[], stri
     config.registry.predeployedProxyAddress,
     config.attestations.attestationExpiryBlocks.toString(),
     config.attestations.selectIssuersWaitBlocks.toString(),
+    config.attestations.maxAttestations.toString(),
     [stableToken.address],
     [attestationFee.toString()],
   ]

packages/protocol/migrationsConfig.js

@@ -19,6 +19,7 @@ const DefaultConfig = {
     attestationExpiryBlocks: HOUR / 5, // ~1 hour,
     attestationRequestFeeInDollars: 0.05,
     selectIssuersWaitBlocks: 4,
+    maxAttestations: 100,
   },
   blockchainParameters: {
     gasForNonGoldCurrencies: 50000,

packages/protocol/test/identity/attestations.ts

@@ -80,6 +80,7 @@ contract('Attestations', (accounts: string[]) => {
   const attestationsRequested = 3
   const attestationExpiryBlocks = (60 * 60) / 5
   const selectIssuersWaitBlocks = 4
+  const maxAttestations = 20
   const attestationFee = new BigNumber(web3.utils.toWei('.05', 'ether').toString())
 
   async function getVerificationCodeSignature(
@@ -174,6 +175,7 @@ contract('Attestations', (accounts: string[]) => {
       registry.address,
       attestationExpiryBlocks,
       selectIssuersWaitBlocks,
+      maxAttestations,
       [mockStableToken.address, otherMockStableToken.address],
       [attestationFee, attestationFee]
     )
@@ -204,6 +206,7 @@ contract('Attestations', (accounts: string[]) => {
           registry.address,
           attestationExpiryBlocks,
           selectIssuersWaitBlocks,
+          maxAttestations,
           [mockStableToken.address],
           [attestationFee]
         )
@@ -305,6 +308,33 @@ contract('Attestations', (accounts: string[]) => {
     })
   })
 
+  describe('#setMaxAttestations()', () => {
+    const newMaxAttestations = maxAttestations + 1
+
+    it('should set maxAttestations', async () => {
+      await attestations.setMaxAttestations(newMaxAttestations)
+      assert.equal(await attestations.maxAttestations.call(this), newMaxAttestations)
+    })
+
+    it('should emit the MaxAttestationsSet event', async () => {
+      const response = await attestations.setMaxAttestations(newMaxAttestations)
+      assert.lengthOf(response.logs, 1)
+      const event = response.logs[0]
+      assertLogMatches2(event, {
+        event: 'MaxAttestationsSet',
+        args: { value: new BigNumber(newMaxAttestations) },
+      })
+    })
+
+    it('should revert when set by a non-owner', async () => {
+      await assertRevert(
+        attestations.setMaxAttestations(newMaxAttestations, {
+          from: accounts[1],
+        })
+      )
+    })
+  })
+
   describe('#request()', () => {
     it('should indicate an unselected attestation request', async () => {
       await attestations.request(phoneHash, attestationsRequested, mockStableToken.address)
@@ -525,6 +555,20 @@ contract('Attestations', (accounts: string[]) => {
           })
         })
 
+        describe('when more attestations were requested', () => {
+          beforeEach(async () => {
+            await attestations.selectIssuers(phoneHash)
+            await attestations.request(phoneHash, 8, mockStableToken.address)
+            expectedRequestBlockNumber = await web3.eth.getBlockNumber()
+            const requestBlockNumber = await web3.eth.getBlockNumber()
+            await random.addTestRandomness(requestBlockNumber + selectIssuersWaitBlocks, '0x1')
+          })
+
+          it('should revert if too many issuers attempted', async () => {
+            await assertRevert(attestations.selectIssuers(phoneHash))
+          })
+        })
+
         describe('after attestationExpiryBlocks', () => {
           beforeEach(async () => {
             await attestations.selectIssuers(phoneHash)