fix: Fix malleable signature (SC-5066) (#26)

* fix: prevent signature malleability * test: test_permitBadV and test_permitBadS * fix: add break
maple-labs · Mar 10, 2022 · 2f5d53c · 2f5d53c
1 parent fd0cfde
commit 2f5d53c
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 4 deletions.
diff --git a/contracts/ERC20.sol b/contracts/ERC20.sol
@@ -29,7 +29,8 @@ contract ERC20 is IERC20 {
     /****************/
 
     // PERMIT_TYPEHASH = keccak256("Permit(address owner,address spender,uint256 amount,uint256 nonce,uint256 deadline)");
-    bytes32 public constant override PERMIT_TYPEHASH = 0xfc77c2b9d30fe91687fd39abb7d16fcdfe1472d065740051ab8b13e4bf4a617f;
+    bytes32 public constant override PERMIT_TYPEHASH      = 0xfc77c2b9d30fe91687fd39abb7d16fcdfe1472d065740051ab8b13e4bf4a617f;
+    uint256 public constant S_VALUE_INCLUSIVE_UPPER_BOUND = 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0;
 
     mapping (address => uint256) public override nonces;
 
@@ -72,6 +73,11 @@ contract ERC20 is IERC20 {
                 keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, amount, nonces[owner]++, deadline))
             )
         );
+
+        // Appendix F in the Ethereum Yellow paper (https://ethereum.github.io/yellowpaper/paper.pdf), defines
+        // the valid range for s in (301): 0 < s < secp256k1n ÷ 2 + 1, and for v in (302): v ∈ {27, 28}.
+        require(uint256(s) <= S_VALUE_INCLUSIVE_UPPER_BOUND && (v == 27 || v == 28), "ERC20:P:MALLEABLE");
+
         address recoveredAddress = ecrecover(digest, v, r, s);
         require(recoveredAddress == owner && owner != address(0), "ERC20:P:INVALID_SIGNATURE");
         _approve(owner, spender, amount);

diff --git a/contracts/test/ERC20.t.sol b/contracts/test/ERC20.t.sol
@@ -226,9 +226,6 @@ contract ERC20PermitTest is TestUtils {
         uint256 amount = 10 * WAD;
         ( uint8 v, bytes32 r, bytes32 s ) = _getValidPermitSignature(amount, owner, skOwner, deadline);
 
-        vm.expectRevert(bytes("ERC20:P:INVALID_SIGNATURE"));
-        user.erc20_permit(address(token), address(0), spender, amount, deadline, 17, r, s);  // https://ethereum.stackexchange.com/questions/69328/how-to-get-the-zero-address-from-ecrecover
-
         vm.expectRevert(bytes("ERC20:P:INVALID_SIGNATURE"));
         user.erc20_permit(address(token), address(0), spender, amount, deadline, v, r, s);
     }
@@ -286,6 +283,40 @@ contract ERC20PermitTest is TestUtils {
         user.erc20_permit(address(token), owner, spender, amount, deadline, v, r, s);
     }
 
+    function test_permitBadS() external {
+        uint256 amount = 10 * WAD;
+        ( uint8 v, bytes32 r, bytes32 s ) = _getValidPermitSignature(amount, owner, skOwner, deadline);
+
+        // Send in an s that is above the upper bound.
+        bytes32 badS = bytes32(token.S_VALUE_INCLUSIVE_UPPER_BOUND() + 1);
+        vm.expectRevert(bytes("ERC20:P:MALLEABLE"));
+        user.erc20_permit(address(token), owner, spender, amount, deadline, v, r, badS);
+
+        user.erc20_permit(address(token), owner, spender, amount, deadline, v, r, s);
+    }
+
+    function test_permitBadV() external {
+        uint256 amount = 10 * WAD;
+        ( uint8 v, bytes32 r, bytes32 s ) = _getValidPermitSignature(amount, owner, skOwner, deadline);
+
+        for (uint8 i; i <= type(uint8).max; i++) {
+            if (i == type(uint8).max) {
+                break;
+            } else if (i == 28) {
+                continue;
+            } else if (i == 27) {
+                // Should get past the Malleable require check as 27 or 28 are valid values for s.
+                vm.expectRevert(bytes("ERC20:P:INVALID_SIGNATURE"));
+            } else {
+                vm.expectRevert(bytes("ERC20:P:MALLEABLE"));
+            }
+            user.erc20_permit(address(token), owner, spender, amount, deadline, i, r, s);
+        }
+
+        assertEq(v, 28);
+        user.erc20_permit(address(token), owner, spender, amount, deadline, v, r, s);
+    }
+
     // Returns an ERC-2612 `permit` digest for the `owner` to sign
     function _getDigest(address owner_, address spender_, uint256 value_, uint256 nonce_, uint256 deadline_) internal view returns (bytes32) {
         return keccak256(