From 199a00aa55bcf786a45ae37708d18af3e1df82e4 Mon Sep 17 00:00:00 2001 From: kevincheng96 Date: Thu, 29 Jun 2023 13:04:27 -0700 Subject: [PATCH] Add tests for EIP1271 encumberBySig --- src/EncumberableToken.sol | 6 +- src/test/EIP1271Signer.sol | 66 ++++++++ test/EncumberBySig.t.sol | 333 ++++++++++++++++++++++++++++++++++++- 3 files changed, 402 insertions(+), 3 deletions(-) create mode 100644 src/test/EIP1271Signer.sol diff --git a/src/EncumberableToken.sol b/src/EncumberableToken.sol index bee3bda..a77d02a 100644 --- a/src/EncumberableToken.sol +++ b/src/EncumberableToken.sol @@ -254,6 +254,8 @@ contract EncumberableToken is ERC20, IERC20Permit, IERC7246 { if (isValidSignature(owner, digest, v, r, s)) { nonces[owner]++; _approve(owner, spender, amount); + } else { + revert("Bad signatory"); } } @@ -283,6 +285,8 @@ contract EncumberableToken is ERC20, IERC20Permit, IERC7246 { if (isValidSignature(owner, digest, v, r, s)) { nonces[owner]++; _encumber(owner, taker, amount); + } else { + revert("Bad signatory"); } } @@ -308,7 +312,7 @@ contract EncumberableToken is ERC20, IERC20Permit, IERC7246 { (bool success, bytes memory data) = signer.staticcall( abi.encodeWithSelector(EIP1271_MAGIC_VALUE, digest, signature) ); - require(success == true, "Call to verify signature failed"); + require(success == true, "Call to verify EIP1271 signature failed"); bytes4 returnValue = abi.decode(data, (bytes4)); return returnValue == EIP1271_MAGIC_VALUE; } else { diff --git a/src/test/EIP1271Signer.sol b/src/test/EIP1271Signer.sol new file mode 100644 index 0000000..adab6ff --- /dev/null +++ b/src/test/EIP1271Signer.sol @@ -0,0 +1,66 @@ +// SPDX-License-Identifier: BUSL-1.1 +pragma solidity ^0.8.15; + +contract EIP1271Signer { + bytes4 internal constant EIP1271_MAGIC_VALUE = 0x1626ba7e; + + address public owner; + + constructor(address _owner) { + owner = _owner; + } + + function isValidSignature(bytes32 messageHash, bytes memory signature) external view returns (bytes4) { + if (recoverSigner(messageHash, signature) == owner) { + return EIP1271_MAGIC_VALUE; + } else { + return 0xffffffff; + } + } + + function recoverSigner(bytes32 messageHash, bytes memory signature) internal pure returns (address) { + require(signature.length == 65, "SignatureValidator#recoverSigner: invalid signature length"); + + // Variables are not scoped in Solidity. + bytes32 r; + bytes32 s; + uint8 v; + assembly { + r := mload(add(signature, 32)) + s := mload(add(signature, 64)) + v := and(mload(add(signature, 65)), 255) + } + + // EIP-2 still allows signature malleability for ecrecover(). Remove this possibility and make the signature + // unique. Appendix F in the Ethereum Yellow paper (https://ethereum.github.io/yellowpaper/paper.pdf), defines + // the valid range for s in (281): 0 < s < secp256k1n ÷ 2 + 1, and for v in (282): v ∈ {27, 28}. Most + // signatures from current libraries generate a unique signature with an s-value in the lower half order. + // + // If your library generates malleable signatures, such as s-values in the upper range, calculate a new s-value + // with 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141 - s1 and flip v from 27 to 28 or + // vice versa. If your library also generates signatures with 0/1 for v instead 27/28, add 27 to v to accept + // these malleable signatures as well. + // + // Source OpenZeppelin + // https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/cryptography/ECDSA.sol + + if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) { + revert("SignatureValidator#recoverSigner: invalid signature 's' value"); + } + + if (v != 27 && v != 28) { + revert("SignatureValidator#recoverSigner: invalid signature 'v' value"); + } + + // Recover ECDSA signer + address signer = ecrecover(messageHash, v, r, s); + + // Prevent signer from being 0x0 + require( + signer != address(0x0), + "SignatureValidator#recoverSigner: INVALID_SIGNER" + ); + + return signer; + } +} \ No newline at end of file diff --git a/test/EncumberBySig.t.sol b/test/EncumberBySig.t.sol index d6e4df8..462dafb 100644 --- a/test/EncumberBySig.t.sol +++ b/test/EncumberBySig.t.sol @@ -5,6 +5,7 @@ import "forge-std/StdUtils.sol"; import "../src/vendor/ERC20.sol"; import "../src/vendor/IERC20Metadata.sol"; import "../src/EncumberableToken.sol"; +import "../src/test/EIP1271Signer.sol"; contract EncumberBySigTest is Test { ERC20 public underlyingToken; @@ -12,16 +13,18 @@ contract EncumberBySigTest is Test { uint256 alicePrivateKey = 0xa11ce; address alice; // see setup() + address aliceContract; address bob = address(11); address charlie = address(12); bytes32 internal constant ENCUMBER_TYPEHASH = keccak256("Encumber(address owner,address taker,uint256 amount,uint256 nonce,uint256 expiry)"); function setUp() public { + alice = vm.addr(alicePrivateKey); + underlyingToken = new ERC20("TEST TOKEN", "TTKN"); wrappedToken = new EncumberableToken(address(underlyingToken)); - - alice = vm.addr(alicePrivateKey); + aliceContract = address(new EIP1271Signer(alice)); } function aliceAuthorization(uint256 amount, uint256 nonce, uint256 expiry) internal view returns (uint8, bytes32, bytes32) { @@ -30,6 +33,12 @@ contract EncumberBySigTest is Test { return vm.sign(alicePrivateKey, digest); } + function aliceContractAuthorization(uint256 amount, uint256 nonce, uint256 expiry) internal view returns (uint8, bytes32, bytes32) { + bytes32 structHash = keccak256(abi.encode(ENCUMBER_TYPEHASH, aliceContract, bob, amount, nonce, expiry)); + bytes32 digest = keccak256(abi.encodePacked("\x19\x01", wrappedToken.DOMAIN_SEPARATOR(), structHash)); + return vm.sign(alicePrivateKey, digest); + } + function testEncumberBySig() public { uint256 aliceBalance = 100e18; uint256 encumbranceAmount = 60e18; @@ -379,4 +388,324 @@ contract EncumberBySigTest is Test { // alice's nonce is not incremented assertEq(wrappedToken.nonces(alice), nonce); } + + /* ===== EIP1271 Tests ===== */ + + function testEncumberBySigEIP1271() public { + uint256 aliceBalance = 100e18; + uint256 encumbranceAmount = 60e18; + + // alice's contract has 100 wrapped tokens + deal(address(wrappedToken), aliceContract, aliceBalance); + + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), 0); + assertEq(wrappedToken.encumbrances(aliceContract, bob), 0); + + uint256 nonce = wrappedToken.nonces(aliceContract); + uint256 expiry = block.timestamp + 1000; + + (uint8 v, bytes32 r, bytes32 s) = aliceContractAuthorization(encumbranceAmount, nonce, expiry); + + // bob calls encumberBySig with the signature + vm.prank(bob); + wrappedToken.encumberBySig(aliceContract, bob, encumbranceAmount, expiry, v, r, s); + + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance - encumbranceAmount); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), encumbranceAmount); + assertEq(wrappedToken.encumbrances(aliceContract, bob), encumbranceAmount); + + // alice's contract's nonce is incremented + assertEq(wrappedToken.nonces(aliceContract), nonce + 1); + } + + function testEncumberBySigRevertsForBadSpenderEIP1271() public { + uint256 aliceBalance = 100e18; + uint256 encumbranceAmount = 60e18; + + // alice's contract has 100 wrapped tokens + deal(address(wrappedToken), aliceContract, aliceBalance); + + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), 0); + assertEq(wrappedToken.encumbrances(aliceContract, bob), 0); + + uint256 nonce = wrappedToken.nonces(aliceContract); + uint256 expiry = block.timestamp + 1000; + + (uint8 v, bytes32 r, bytes32 s) = aliceContractAuthorization(encumbranceAmount, nonce, expiry); + + // bob calls encumberBySig with the signature, but he manipulates the spender + vm.prank(bob); + vm.expectRevert("Bad signatory"); + wrappedToken.encumberBySig(aliceContract, charlie, encumbranceAmount, expiry, v, r, s); + + // no encumbrance is created + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), 0); + assertEq(wrappedToken.encumbrances(aliceContract, bob), 0); + + // alice's contract's nonce is not incremented + assertEq(wrappedToken.nonces(aliceContract), nonce); + } + + function testEncumberBySigRevertsForBadAmountEIP1271() public { + uint256 aliceBalance = 100e18; + uint256 encumbranceAmount = 60e18; + + // alice's contract has 100 wrapped tokens + deal(address(wrappedToken), aliceContract, aliceBalance); + + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), 0); + assertEq(wrappedToken.encumbrances(aliceContract, bob), 0); + + uint256 nonce = wrappedToken.nonces(aliceContract); + uint256 expiry = block.timestamp + 1000; + + (uint8 v, bytes32 r, bytes32 s) = aliceContractAuthorization(encumbranceAmount, nonce, expiry); + + // bob calls encumberBySig with the signature, but he manipulates the encumbranceAmount + vm.prank(bob); + vm.expectRevert("Bad signatory"); + wrappedToken.encumberBySig(aliceContract, bob, encumbranceAmount + 1 wei, expiry, v, r, s); + + // no encumbrance is created + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), 0); + assertEq(wrappedToken.encumbrances(aliceContract, bob), 0); + + // alice's contract's nonce is not incremented + assertEq(wrappedToken.nonces(aliceContract), nonce); + } + + function testEncumberBySigRevertsForBadExpiryEIP1271() public { + uint256 aliceBalance = 100e18; + uint256 encumbranceAmount = 60e18; + + // alice's contract has 100 wrapped tokens + deal(address(wrappedToken), aliceContract, aliceBalance); + + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), 0); + assertEq(wrappedToken.encumbrances(aliceContract, bob), 0); + + uint256 nonce = wrappedToken.nonces(aliceContract); + uint256 expiry = block.timestamp + 1000; + + (uint8 v, bytes32 r, bytes32 s) = aliceContractAuthorization(encumbranceAmount, nonce, expiry); + + // bob calls encumberBySig with the signature, but he manipulates the expiry + vm.prank(bob); + vm.expectRevert("Bad signatory"); + wrappedToken.encumberBySig(aliceContract, bob, encumbranceAmount, expiry + 1, v, r, s); + + // no encumbrance is created + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), 0); + assertEq(wrappedToken.encumbrances(aliceContract, bob), 0); + + // alice's contract's nonce is not incremented + assertEq(wrappedToken.nonces(aliceContract), nonce); + } + + function testEncumberBySigRevertsForBadNonceEIP1271() public { + uint256 aliceBalance = 100e18; + uint256 encumbranceAmount = 60e18; + + // alice's contract has 100 wrapped tokens + deal(address(wrappedToken), aliceContract, aliceBalance); + + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), 0); + assertEq(wrappedToken.encumbrances(aliceContract, bob), 0); + + // alice signs an authorization with an invalid nonce + uint256 nonce = wrappedToken.nonces(aliceContract); + uint256 badNonce = nonce + 1; + uint256 expiry = block.timestamp + 1000; + + (uint8 v, bytes32 r, bytes32 s) = aliceContractAuthorization(encumbranceAmount, badNonce, expiry); + + // bob calls encumberBySig with the signature with an invalid nonce + vm.prank(bob); + vm.expectRevert("Bad signatory"); + wrappedToken.encumberBySig(aliceContract, bob, encumbranceAmount, expiry, v, r, s); + + // no encumbrance is created + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), 0); + assertEq(wrappedToken.encumbrances(aliceContract, bob), 0); + + // alice's contract's nonce is not incremented + assertEq(wrappedToken.nonces(aliceContract), nonce); + } + + function testEncumberBySigRevertsOnRepeatedCallEIP1271() public { + uint256 aliceBalance = 100e18; + uint256 encumbranceAmount = 60e18; + uint256 transferAmount = 30e18; + + // alice's contract has 100 wrapped tokens + deal(address(wrappedToken), aliceContract, aliceBalance); + + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), 0); + assertEq(wrappedToken.encumbrances(aliceContract, bob), 0); + + uint256 nonce = wrappedToken.nonces(alice); + uint256 expiry = block.timestamp + 1000; + + (uint8 v, bytes32 r, bytes32 s) = aliceContractAuthorization(encumbranceAmount, nonce, expiry); + + // bob calls encumberBySig with the signature + vm.startPrank(bob); + wrappedToken.encumberBySig(aliceContract, bob, encumbranceAmount, expiry, v, r, s); + + // the encumbrance is created + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance - encumbranceAmount); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), encumbranceAmount); + assertEq(wrappedToken.encumbrances(aliceContract, bob), encumbranceAmount); + + // alice's contract's nonce is incremented + assertEq(wrappedToken.nonces(aliceContract), nonce + 1); + + // bob uses some of the encumbrance to transfer to himself + wrappedToken.transferFrom(aliceContract, bob, transferAmount); + + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance - transferAmount); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance - encumbranceAmount); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), encumbranceAmount - transferAmount); + assertEq(wrappedToken.encumbrances(aliceContract, bob), encumbranceAmount - transferAmount); + + // bob tries to reuse the same signature twice + vm.expectRevert("Bad signatory"); + wrappedToken.encumberBySig(aliceContract, bob, encumbranceAmount, expiry, v, r, s); + + // no new encumbrance is created + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance - transferAmount); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance - encumbranceAmount); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), encumbranceAmount - transferAmount); + assertEq(wrappedToken.encumbrances(aliceContract, bob), encumbranceAmount - transferAmount); + + // alice's contract's nonce is not incremented a second time + assertEq(wrappedToken.nonces(aliceContract), nonce + 1); + + vm.stopPrank(); + } + + function testEncumberBySigRevertsForExpiredSignatureEIP1271() public { + uint256 aliceBalance = 100e18; + uint256 encumbranceAmount = 60e18; + + // alice's contract has 100 wrapped tokens + deal(address(wrappedToken), aliceContract, aliceBalance); + + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), 0); + assertEq(wrappedToken.encumbrances(aliceContract, bob), 0); + + uint256 nonce = wrappedToken.nonces(aliceContract); + uint256 expiry = block.timestamp + 1000; + + (uint8 v, bytes32 r, bytes32 s) = aliceContractAuthorization(encumbranceAmount, nonce, expiry); + + // the expiry block arrives + vm.warp(expiry); + + // bob calls encumberBySig with the signature after the expiry + vm.prank(bob); + vm.expectRevert("Signature expired"); + wrappedToken.encumberBySig(aliceContract, bob, encumbranceAmount, expiry, v, r, s); + + // no encumbrance is created + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), 0); + assertEq(wrappedToken.encumbrances(aliceContract, bob), 0); + + // alice's contract's nonce is not incremented + assertEq(wrappedToken.nonces(aliceContract), nonce); + } + + function testEncumberBySigRevertsInvalidVEIP1271() public { + uint256 aliceBalance = 100e18; + uint256 encumbranceAmount = 60e18; + + // alice's contract has 100 wrapped tokens + deal(address(wrappedToken), aliceContract, aliceBalance); + + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), 0); + assertEq(wrappedToken.encumbrances(aliceContract, bob), 0); + + uint256 nonce = wrappedToken.nonces(aliceContract); + uint256 expiry = block.timestamp + 1000; + + (, bytes32 r, bytes32 s) = aliceContractAuthorization(encumbranceAmount, nonce, expiry); + uint8 invalidV = 26; + + // bob calls encumberBySig with the signature with an invalid `v` value + vm.prank(bob); + vm.expectRevert("Call to verify EIP1271 signature failed"); + wrappedToken.encumberBySig(aliceContract, bob, encumbranceAmount, expiry, invalidV, r, s); + + // no encumbrance is created + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), 0); + assertEq(wrappedToken.encumbrances(aliceContract, bob), 0); + + // alice's contract's nonce is not incremented + assertEq(wrappedToken.nonces(aliceContract), nonce); + } + + function testEncumberBySigRevertsInvalidSEIP1271() public { + uint256 aliceBalance = 100e18; + uint256 encumbranceAmount = 60e18; + + // alice's contract has 100 wrapped tokens + deal(address(wrappedToken), aliceContract, aliceBalance); + + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), 0); + assertEq(wrappedToken.encumbrances(aliceContract, bob), 0); + + uint256 nonce = wrappedToken.nonces(aliceContract); + uint256 expiry = block.timestamp + 1000; + + (uint8 v, bytes32 r, ) = aliceContractAuthorization(encumbranceAmount, nonce, expiry); + + // 1 greater than the max value of s + bytes32 invalidS = 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A1; + + // bob calls encumberBySig with the signature, but he manipulates the expiry + vm.prank(bob); + vm.expectRevert("Call to verify EIP1271 signature failed"); + wrappedToken.encumberBySig(aliceContract, bob, encumbranceAmount, expiry, v, r, invalidS); + + // no encumbrance is created + assertEq(wrappedToken.balanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.availableBalanceOf(aliceContract), aliceBalance); + assertEq(wrappedToken.encumberedBalanceOf(aliceContract), 0); + assertEq(wrappedToken.encumbrances(aliceContract, bob), 0); + + // alice's contract's nonce is not incremented + assertEq(wrappedToken.nonces(aliceContract), nonce); + } }