From fe95c4ba07b7cb29a8407bf46db707efeba0a1a8 Mon Sep 17 00:00:00 2001 From: Erick Date: Mon, 14 Mar 2022 14:50:52 -0400 Subject: [PATCH 1/3] fix: test_permitBadV --- contracts/test/ERC20.t.sol | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/contracts/test/ERC20.t.sol b/contracts/test/ERC20.t.sol index bbe6dd2..67f1b49 100644 --- a/contracts/test/ERC20.t.sol +++ b/contracts/test/ERC20.t.sol @@ -302,18 +302,20 @@ contract ERC20PermitTest is TestUtils { 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 { + } else if (i != 27 && i != 28) { vm.expectRevert(bytes("ERC20:P:MALLEABLE")); + } else { + if (i == v) { + continue; + } else { + + // Should get past the Malleable require check as 27 or 28 are valid values for s. + vm.expectRevert(bytes("ERC20:P:INVALID_SIGNATURE")); + } } 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); } From a39c50c110e2e148e2d4cc69c202bad954711261 Mon Sep 17 00:00:00 2001 From: Erick Date: Mon, 14 Mar 2022 15:26:20 -0400 Subject: [PATCH 2/3] style: remove extra line --- contracts/test/ERC20.t.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/contracts/test/ERC20.t.sol b/contracts/test/ERC20.t.sol index 67f1b49..3d46a24 100644 --- a/contracts/test/ERC20.t.sol +++ b/contracts/test/ERC20.t.sol @@ -308,7 +308,6 @@ contract ERC20PermitTest is TestUtils { if (i == v) { continue; } else { - // Should get past the Malleable require check as 27 or 28 are valid values for s. vm.expectRevert(bytes("ERC20:P:INVALID_SIGNATURE")); } From 12fcb088a74462508b2d0e3ac35860008cec88f3 Mon Sep 17 00:00:00 2001 From: Erick Date: Mon, 14 Mar 2022 15:29:53 -0400 Subject: [PATCH 3/3] chore: add comment explaining the test --- contracts/test/ERC20.t.sol | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contracts/test/ERC20.t.sol b/contracts/test/ERC20.t.sol index 3d46a24..d29606a 100644 --- a/contracts/test/ERC20.t.sol +++ b/contracts/test/ERC20.t.sol @@ -297,6 +297,10 @@ contract ERC20PermitTest is TestUtils { function test_permitBadV() external { uint256 amount = 10 * WAD; + + // Get valid signature. The `v` value is the expected v value that will cause `permit` to succeed, and must be 27 or 28. + // Any other value should fail. + // If v is 27, then 28 should make it past the MALLEABLE require, but should result in an invalid signature, and vice versa when v is 28. ( uint8 v, bytes32 r, bytes32 s ) = _getValidPermitSignature(amount, owner, skOwner, deadline); for (uint8 i; i <= type(uint8).max; i++) {