Merge pull request #37 from hellobloom/fix/TxMalleability

fix signature malleability issue
hellobloom · Nov 27, 2018 · de67de3 · de67de3
2 parents 76750b9 + d824c07
commit de67de3
Show file tree

Hide file tree

Showing 11 changed files with 163 additions and 177 deletions.
diff --git a/README.md b/README.md
@@ -103,7 +103,6 @@ Links are stored as mapping from addresses to a link Id. Users can have any numb
 ## Account Registry Logic Public Functions
 ### linkIds
 Public getter to check if an address is linked to another address. Returns 0 if no link.
-Retrieve the account Id associated with an address. Reverts if the address is not associated with an account
 #### Interface
 ```
 function linkIds(address _address) public view returns (uint256)
@@ -122,7 +121,7 @@ if (linkA === linkB && linkA !== 0) {
 ```
 
 ### linkAddresses
-Store a link demonstrating ownership of multiple addresses. Each address much sign a message indicating intention to be linked. `currentAddress` may be linked to other addresses. `newAddress` must be unclaimed.
+Store a link demonstrating ownership of multiple addresses. Each address must sign a message indicating intention to be linked. `currentAddress` may be linked to other addresses. `newAddress` must be unclaimed.
 
 A user can submit this transaction from their own address or it can be submitted by a third party on their behalf.
 #### Interface

diff --git a/contracts/AccountRegistryLogic.sol b/contracts/AccountRegistryLogic.sol
@@ -29,7 +29,7 @@ contract AccountRegistryLogic is Initializable, SigningLogic {
    * @param _currentAddressSig Signed message from address currently associated with account confirming intention
    * @param _newAddress Address to add to account. Cannot currently be linked to another address
    * @param _newAddressSig Signed message from new address confirming ownership by the sender
-   * @param _nonce uuid used when generating sigs to make them one time use
+   * @param _nonce hex string used when generating sigs to make them one time use
    */
   function linkAddresses(
     address _currentAddress,
@@ -42,11 +42,9 @@ contract AccountRegistryLogic is Initializable, SigningLogic {
       require(linkIds[_newAddress] == 0);
       // Confirm new address is signed by current address and is unused
       validateLinkSignature(_currentAddress, _newAddress, _nonce, _currentAddressSig);
-      burnSignature(_currentAddressSig);
 
       // Confirm current address is signed by new address and is unused
       validateLinkSignature(_newAddress, _currentAddress, _nonce, _newAddressSig);
-      burnSignature(_newAddressSig);
 
       // Get linkId of current address if exists. Otherwise use incremented linkCounter
       if (linkIds[_currentAddress] == 0) {
@@ -61,7 +59,7 @@ contract AccountRegistryLogic is Initializable, SigningLogic {
    * @notice Remove an address from a link relationship
    * @param _addressToRemove Address to unlink from all other addresses
    * @param _unlinkSignature Signed message from address currently associated with account confirming intention to unlink
-   * @param _nonce uuid used when generating sigs to make them one time use
+   * @param _nonce hex string used when generating sigs to make them one time use
    */
   function unlinkAddress(
     address _addressToRemove,
@@ -70,30 +68,27 @@ contract AccountRegistryLogic is Initializable, SigningLogic {
   ) public {
     // Confirm unlink request is signed by sender and is unused
     validateUnlinkSignature(_addressToRemove, _nonce, _unlinkSignature);
-    burnSignature(_unlinkSignature);
     linkIds[_addressToRemove] = 0;
 
     emit AddressUnlinked(_addressToRemove);
   }
 
   /**
    * @notice Verify link signature is valid and unused V
-   * @param _addressA Address signing intention to link
-   * @param _addressB Address being linked
+   * @param _currentAddress Address signing intention to link
+   * @param _addressToAdd Address being linked
    * @param _nonce Unique nonce for this request
    * @param _linkSignature Signature of address a
    */
   function validateLinkSignature(
-    address _addressA,
-    address _addressB,
+    address _currentAddress,
+    address _addressToAdd,
     bytes32 _nonce,
     bytes _linkSignature
   ) internal view {
-    require(_addressA == recoverSigner(
-      generateAddAddressSchemaHash(
-      _addressB,
-      _nonce
-    ), _linkSignature));
+    bytes32 _signatureDigest = generateAddAddressSchemaHash(_addressToAdd, _nonce);
+    require(_currentAddress == recoverSigner(_signatureDigest, _linkSignature));
+    burnSignatureDigest(_signatureDigest, _currentAddress);
   }
 
   /**
@@ -111,11 +106,10 @@ contract AccountRegistryLogic is Initializable, SigningLogic {
     // require that address to remove is currently linked to senderAddress
     require(linkIds[_addressToRemove] != 0, "Address does not have active link");
 
-    require(_addressToRemove == recoverSigner(
-      generateRemoveAddressSchemaHash(
-      _addressToRemove,
-      _nonce
-    ), _unlinkSignature));
+    bytes32 _signatureDigest = generateRemoveAddressSchemaHash(_addressToRemove, _nonce);
+
+    require(_addressToRemove == recoverSigner(_signatureDigest, _unlinkSignature));
+    burnSignatureDigest(_signatureDigest, _addressToRemove);
   }
 
   /**

diff --git a/contracts/AttestationLogic.sol b/contracts/AttestationLogic.sol
@@ -92,7 +92,6 @@ contract AttestationLogic is Initializable, SigningLogic{
   ) public {
     // Confirm attester address matches recovered address from signature
     validateAttestForSig(_subject, _attester, _requester, _reward, _dataHash, _requestNonce, _delegationSig);
-    burnSignature(_delegationSig);
     attestForUser(
       _subject,
       _attester,
@@ -134,7 +133,6 @@ contract AttestationLogic is Initializable, SigningLogic{
       _requestNonce,
       _subjectSig
     );
-    burnSignature(_subjectSig);
 
     emit TraitAttested(
       _subject,
@@ -196,7 +194,6 @@ contract AttestationLogic is Initializable, SigningLogic{
       _requestNonce,
       _delegationSig
     );
-    burnSignature(_delegationSig);
     contestForUser(
       _attester,
       _requester,
@@ -242,11 +239,9 @@ contract AttestationLogic is Initializable, SigningLogic{
     bytes32 _requestNonce,
     bytes _subjectSig
   ) internal view {
-    require(_subject == recoverSigner(
-      generateRequestAttestationSchemaHash(
-      _dataHash,
-      _requestNonce
-    ), _subjectSig));
+    bytes32 _signatureDigest = generateRequestAttestationSchemaHash(_dataHash, _requestNonce);
+    require(_subject == recoverSigner(_signatureDigest, _subjectSig));
+    burnSignatureDigest(_signatureDigest, _subject);
   }
 
   /**
@@ -268,15 +263,9 @@ contract AttestationLogic is Initializable, SigningLogic{
     bytes32 _requestNonce,
     bytes _delegationSig
   ) internal view {
-    require(_attester == recoverSigner(
-      generateAttestForDelegationSchemaHash(
-        _subject,
-        _requester,
-        _reward,
-        _dataHash,
-        _requestNonce
-        ),
-        _delegationSig), 'Invalid AttestFor Signature');
+    bytes32 _delegationDigest = generateAttestForDelegationSchemaHash(_subject, _requester, _reward, _dataHash, _requestNonce);
+    require(_attester == recoverSigner(_delegationDigest, _delegationSig), 'Invalid AttestFor Signature');
+    burnSignatureDigest(_delegationDigest, _attester);
   }
 
   /**
@@ -294,13 +283,9 @@ contract AttestationLogic is Initializable, SigningLogic{
     bytes32 _requestNonce,
     bytes _delegationSig
   ) internal view {
-    require(_attester == recoverSigner(
-      generateContestForDelegationSchemaHash(
-        _requester,
-        _reward,
-        _requestNonce
-        ),
-        _delegationSig), 'Invalid ContestFor Signature');
+    bytes32 _delegationDigest = generateContestForDelegationSchemaHash(_requester, _reward, _requestNonce);
+    require(_attester == recoverSigner(_delegationDigest, _delegationSig), 'Invalid ContestFor Signature');
+    burnSignatureDigest(_delegationDigest, _attester);
   }
 
   /**
@@ -342,12 +327,12 @@ contract AttestationLogic is Initializable, SigningLogic{
    * @param _link bytes string embedded in dataHash to link revocation
    */
   function revokeAttestationFor(
-    bytes32 _link,
     address _sender,
+    bytes32 _link,
+    bytes32 _nonce,
     bytes _delegationSig
     ) public {
-      validateRevokeForSig(_link, _sender, _delegationSig);
-      burnSignature(_delegationSig);
+      validateRevokeForSig(_sender, _link, _nonce, _delegationSig);
       revokeAttestationForUser(_link, _sender);
   }
 
@@ -358,15 +343,14 @@ contract AttestationLogic is Initializable, SigningLogic{
    * @param _delegationSig signature authorizing revocation on behalf of revoker
    */
   function validateRevokeForSig(
-    bytes32 _link,
     address _sender,
+    bytes32 _link,
+    bytes32 _nonce,
     bytes _delegationSig
   ) internal view {
-    require(_sender == recoverSigner(
-      generateRevokeAttestationForDelegationSchemaHash(
-        _link
-        ),
-        _delegationSig), 'Invalid RevokeFor Signature');
+    bytes32 _delegationDigest = generateRevokeAttestationForDelegationSchemaHash(_link, _nonce);
+    require(_sender == recoverSigner(_delegationDigest, _delegationSig), 'Invalid RevokeFor Signature');
+    burnSignatureDigest(_delegationDigest, _sender);
   }
 
   /**

diff --git a/contracts/Poll.sol b/contracts/Poll.sol
@@ -47,17 +47,21 @@ contract Poll is DependentOnIPFS, SigningLogic {
   }
 
   function voteFor(uint16 _choice, address _voter, bytes32 _nonce, bytes _delegationSig) external {
-    require(_voter == recoverSigner(
-      generateVoteForDelegationSchemaHash(
-        _choice,
-        _voter,
-        _nonce,
-        this),
-      _delegationSig),
+    validateVoteForSig(_choice, _voter, _nonce, _delegationSig);
+    voteForUser(_choice, _voter);
+  }
+
+  function validateVoteForSig(
+    uint16 _choice,
+    address _voter,
+    bytes32 _nonce,
+    bytes _delegationSig
+  ) internal view {
+    bytes32 _signatureDigest = generateVoteForDelegationSchemaHash(_choice, _voter, _nonce, this);
+    require(_voter == recoverSigner(_signatureDigest, _delegationSig),
       "Invalid signer"
       );
-    burnSignature(_delegationSig);
-    voteForUser(_choice, _voter);
+    burnSignatureDigest(_signatureDigest, _voter);
   }
 
   /**

diff --git a/contracts/SigningLogic.sol b/contracts/SigningLogic.sol
@@ -13,10 +13,10 @@ contract SigningLogic {
   //  have been used so they can't be replayed
   mapping (bytes32 => bool) public usedSignatures;
 
-  function burnSignature(bytes _signature) internal {
-    bytes32 _signatureHash = keccak256(abi.encodePacked(_signature));
-    require(!usedSignatures[_signatureHash], "Signature not unique");
-    usedSignatures[_signatureHash] = true;
+  function burnSignatureDigest(bytes32 _signatureDigest, address _sender) internal {
+    bytes32 _txDataHash = keccak256(abi.encode(_signatureDigest, _sender));
+    require(!usedSignatures[_txDataHash], "Signature not unique");
+    usedSignatures[_txDataHash] = true;
   }
 
   bytes32 constant EIP712DOMAIN_TYPEHASH = keccak256(
@@ -52,7 +52,7 @@ contract SigningLogic {
   );
 
   bytes32 constant REVOKE_ATTESTATION_FOR_TYPEHASH = keccak256(
-    "RevokeAttestationFor(bytes32 link)"
+    "RevokeAttestationFor(bytes32 link,bytes32 nonce)"
   );
 
   bytes32 constant VOTE_FOR_TYPEHASH = keccak256(
@@ -183,12 +183,14 @@ contract SigningLogic {
 
   struct RevokeAttestationFor {
       bytes32 link;
+      bytes32 nonce;
   }
 
   function hash(RevokeAttestationFor request) internal pure returns (bytes32) {
     return keccak256(abi.encode(
       REVOKE_ATTESTATION_FOR_TYPEHASH,
-      request.link
+      request.link,
+      request.nonce
     ));
   }
 
@@ -348,14 +350,16 @@ contract SigningLogic {
   }
 
   function generateRevokeAttestationForDelegationSchemaHash(
-    bytes32 _link
+    bytes32 _link,
+    bytes32 _nonce
   ) internal view returns (bytes32) {
     return keccak256(
       abi.encodePacked(
         "\x19\x01",
         DOMAIN_SEPARATOR,
         hash(RevokeAttestationFor(
-          _link
+          _link,
+          _nonce
         ))
       )
       );

diff --git a/contracts/TokenEscrowMarketplace.sol b/contracts/TokenEscrowMarketplace.sol
@@ -72,7 +72,6 @@ contract TokenEscrowMarketplace is SigningLogic {
         _nonce,
         _delegationSig
       );
-      burnSignature(_delegationSig);
       moveTokensToEscrowLockupForUser(_sender, _amount);
   }
 
@@ -90,13 +89,9 @@ contract TokenEscrowMarketplace is SigningLogic {
     bytes _delegationSig
 
   ) internal view {
-    require(_sender == recoverSigner(
-      generateLockupTokensDelegationSchemaHash(
-        _sender,
-        _amount,
-        _nonce
-        ),
-        _delegationSig), 'Invalid LockupTokens Signature');
+    bytes32 _signatureDigest = generateLockupTokensDelegationSchemaHash(_sender, _amount, _nonce);
+    require(_sender == recoverSigner(_signatureDigest, _delegationSig), 'Invalid LockupTokens Signature');
+    burnSignatureDigest(_signatureDigest, _sender);
   }
 
   /**
@@ -143,7 +138,6 @@ contract TokenEscrowMarketplace is SigningLogic {
         _nonce,
         _delegationSig
       );
-      burnSignature(_delegationSig);
       releaseTokensFromEscrowForUser(_sender, _amount);
   }
 
@@ -161,13 +155,9 @@ contract TokenEscrowMarketplace is SigningLogic {
     bytes _delegationSig
 
   ) internal view {
-    require(_sender == recoverSigner(
-      generateReleaseTokensDelegationSchemaHash(
-        _sender,
-        _amount,
-        _nonce
-        ),
-        _delegationSig), 'Invalid ReleaseTokens Signature');
+    bytes32 _signatureDigest = generateReleaseTokensDelegationSchemaHash(_sender, _amount, _nonce);
+    require(_sender == recoverSigner(_signatureDigest, _delegationSig), 'Invalid ReleaseTokens Signature');
+    burnSignatureDigest(_signatureDigest, _sender);
   }
 
   /**
@@ -229,8 +219,6 @@ contract TokenEscrowMarketplace is SigningLogic {
       _nonce,
       _paymentSig
     );
-    burnSignature(_paymentSig);
-
     payTokensFromEscrow(_payer, _receiver, _amount);
     emit TokenMarketplaceEscrowPayment(_payer, _receiver, _amount);
   }
@@ -251,14 +239,9 @@ contract TokenEscrowMarketplace is SigningLogic {
     bytes _paymentSig
 
   ) internal view {
-    require(_payer == recoverSigner(
-      generatePayTokensSchemaHash(
-        _payer,
-        _receiver,
-        _amount,
-        _nonce
-        ),
-        _paymentSig), 'Invalid Payment Signature');
+    bytes32 _signatureDigest = generatePayTokensSchemaHash(_payer, _receiver, _amount, _nonce);
+    require(_payer == recoverSigner(_signatureDigest, _paymentSig), 'Invalid Payment Signature');
+    burnSignatureDigest(_signatureDigest, _payer);
   }
 
   /**