From 5f5d6c72ab96361f698f7d03452242512b881a9c Mon Sep 17 00:00:00 2001 From: Michael De Luca <35537333+deluca-mike@users.noreply.github.com> Date: Wed, 16 Mar 2022 00:38:07 -0400 Subject: [PATCH] chore: Standardization and cleanup (#31) * chore: Standardization and cleanup * feat: add testFuzz to all fuzz tests * chore: improved tests Co-authored-by: Lucas Manuel --- contracts/ERC20.sol | 20 +- contracts/interfaces/IERC20.sol | 20 +- contracts/test/ERC20.t.sol | 389 ++++++++++++++------------ contracts/test/accounts/ERC20User.sol | 8 +- contracts/test/mocks/MockERC20.sol | 8 +- 5 files changed, 235 insertions(+), 210 deletions(-) diff --git a/contracts/ERC20.sol b/contracts/ERC20.sol index 23cdf76..e322fb7 100644 --- a/contracts/ERC20.sol +++ b/contracts/ERC20.sol @@ -29,7 +29,7 @@ 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 = bytes32(0xfc77c2b9d30fe91687fd39abb7d16fcdfe1472d065740051ab8b13e4bf4a617f); mapping(address => uint256) public override nonces; @@ -63,14 +63,14 @@ contract ERC20 is IERC20 { return true; } - function permit(address owner, address spender, uint256 amount, uint256 deadline, uint8 v, bytes32 r, bytes32 s) external override { - require(deadline >= block.timestamp, "ERC20:P:EXPIRED"); + function permit(address owner_, address spender_, uint256 amount_, uint256 deadline_, uint8 v_, bytes32 r_, bytes32 s_) external override { + require(deadline_ >= block.timestamp, "ERC20:P:EXPIRED"); // 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) <= 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0 && - (v == 27 || v == 28), + uint256(s_) <= uint256(0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) && + (v_ == 27 || v_ == 28), "ERC20:P:MALLEABLE" ); @@ -80,14 +80,16 @@ contract ERC20 is IERC20 { abi.encodePacked( "\x19\x01", DOMAIN_SEPARATOR(), - keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, amount, nonces[owner]++, deadline)) + keccak256(abi.encode(PERMIT_TYPEHASH, owner_, spender_, amount_, nonces[owner_]++, deadline_)) ) ); - address recoveredAddress = ecrecover(digest, v, r, s); - require(recoveredAddress == owner && owner != address(0), "ERC20:P:INVALID_SIGNATURE"); + + address recoveredAddress = ecrecover(digest, v_, r_, s_); + + require(recoveredAddress == owner_ && owner_ != address(0), "ERC20:P:INVALID_SIGNATURE"); } - _approve(owner, spender, amount); + _approve(owner_, spender_, amount_); } function transfer(address recipient_, uint256 amount_) external override returns (bool success_) { diff --git a/contracts/interfaces/IERC20.sol b/contracts/interfaces/IERC20.sol index e9d524d..0c68bcd 100644 --- a/contracts/interfaces/IERC20.sol +++ b/contracts/interfaces/IERC20.sol @@ -8,14 +8,6 @@ interface IERC20 { /*** Events ***/ /**************/ - /** - * @dev Emits an event indicating that tokens have moved from one account to another. - * @param owner_ Account that tokens have moved from. - * @param recipient_ Account that tokens have moved to. - * @param amount_ Amount of tokens that have been transferred. - */ - event Transfer(address indexed owner_, address indexed recipient_, uint256 amount_); - /** * @dev Emits an event indicating that one account has set the allowance of another account over their tokens. * @param owner_ Account that tokens are approved from. @@ -24,6 +16,14 @@ interface IERC20 { */ event Approval(address indexed owner_, address indexed spender_, uint256 amount_); + /** + * @dev Emits an event indicating that tokens have moved from one account to another. + * @param owner_ Account that tokens have moved from. + * @param recipient_ Account that tokens have moved to. + * @param amount_ Amount of tokens that have been transferred. + */ + event Transfer(address indexed owner_, address indexed recipient_, uint256 amount_); + /**************************/ /*** External Functions ***/ /**************************/ @@ -133,9 +133,9 @@ interface IERC20 { /** * @dev Returns the permit type hash. - * @return hash_ The permit type hash. + * @return permitTypehash_ The permit type hash. */ - function PERMIT_TYPEHASH() external view returns (bytes32 hash_); + function PERMIT_TYPEHASH() external view returns (bytes32 permitTypehash_); /** * @dev Returns the symbol of the token. diff --git a/contracts/test/ERC20.t.sol b/contracts/test/ERC20.t.sol index 6f38e95..9bf733f 100644 --- a/contracts/test/ERC20.t.sol +++ b/contracts/test/ERC20.t.sol @@ -3,6 +3,8 @@ pragma solidity ^0.8.7; import { InvariantTest, TestUtils } from "../../modules/contract-test-utils/contracts/test.sol"; +import { IERC20 } from "../interfaces/IERC20.sol"; + import { ERC20 } from "../ERC20.sol"; import { ERC20User } from "./accounts/ERC20User.sol"; @@ -10,351 +12,372 @@ import { MockERC20 } from "./mocks/MockERC20.sol"; contract ERC20BaseTest is TestUtils { - bytes constant ARITHMETIC_ERROR = abi.encodeWithSignature("Panic(uint256)", 0x11); + address internal immutable self = address(this); - MockERC20 token; + bytes internal constant ARITHMETIC_ERROR = abi.encodeWithSignature("Panic(uint256)", 0x11); - address internal immutable self = address(this); + MockERC20 internal _token; - function setUp() public virtual { - token = new MockERC20("Token", "TKN", 18); + function setUp() external virtual { + _token = new MockERC20("Token", "TKN", 18); } - function invariant_metadata() public { - assertEq(token.name(), "Token"); - assertEq(token.symbol(), "TKN"); - assertEq(token.decimals(), 18); + function invariant_metadata() external { + assertEq(_token.name(), "Token"); + assertEq(_token.symbol(), "TKN"); + assertEq(_token.decimals(), 18); } - function test_metadata(string memory name, string memory symbol, uint8 decimals) public { - MockERC20 mockToken = new MockERC20(name, symbol, decimals); + function testFuzz_metadata(string memory name_, string memory symbol_, uint8 decimals_) external { + MockERC20 mockToken = new MockERC20(name_, symbol_, decimals_); - assertEq(mockToken.name(), name); - assertEq(mockToken.symbol(), symbol); - assertEq(mockToken.decimals(), decimals); + assertEq(mockToken.name(), name_); + assertEq(mockToken.symbol(), symbol_); + assertEq(mockToken.decimals(), decimals_); } - function test_mint(address account, uint256 amount) public { - token.mint(account, amount); + function testFuzz_mint(address account_, uint256 amount_) external { + _token.mint(account_, amount_); - assertEq(token.totalSupply(), amount); - assertEq(token.balanceOf(account), amount); + assertEq(_token.totalSupply(), amount_); + assertEq(_token.balanceOf(account_), amount_); } - function test_burn(address account, uint256 amount0, uint256 amount1) public { - if (amount1 > amount0) return; // Mint amount must exceed burn amount. + function testFuzz_burn(address account_, uint256 amount0_, uint256 amount1_) external { + if (amount1_ > amount0_) return; // Mint amount must exceed burn amount. - token.mint(account, amount0); - token.burn(account, amount1); + _token.mint(account_, amount0_); + _token.burn(account_, amount1_); - assertEq(token.totalSupply(), amount0 - amount1); - assertEq(token.balanceOf(account), amount0 - amount1); + assertEq(_token.totalSupply(), amount0_ - amount1_); + assertEq(_token.balanceOf(account_), amount0_ - amount1_); } - function test_approve(address account, uint256 amount) public { - assertTrue(token.approve(account, amount)); + function testFuzz_approve(address account_, uint256 amount_) external { + assertTrue(_token.approve(account_, amount_)); - assertEq(token.allowance(self, account), amount); + assertEq(_token.allowance(self, account_), amount_); } - function test_increaseAllowance(address account, uint256 initialAmount, uint256 addedAmount) public { - initialAmount = constrictToRange(initialAmount, 0, type(uint256).max / 2); - addedAmount = constrictToRange(addedAmount, 0, type(uint256).max / 2); + function testFuzz_increaseAllowance(address account_, uint256 initialAmount_, uint256 addedAmount_) external { + initialAmount_ = constrictToRange(initialAmount_, 0, type(uint256).max / 2); + addedAmount_ = constrictToRange(addedAmount_, 0, type(uint256).max / 2); - token.approve(account, initialAmount); + _token.approve(account_, initialAmount_); - assertEq(token.allowance(self, account), initialAmount); + assertEq(_token.allowance(self, account_), initialAmount_); - assertTrue(token.increaseAllowance(account, addedAmount)); + assertTrue(_token.increaseAllowance(account_, addedAmount_)); - assertEq(token.allowance(self, account), initialAmount + addedAmount); + assertEq(_token.allowance(self, account_), initialAmount_ + addedAmount_); } - function test_decreaseAllowance(address account, uint256 initialAmount, uint256 subtractedAmount) public { - initialAmount = constrictToRange(initialAmount, 0, type(uint256).max); - subtractedAmount = constrictToRange(subtractedAmount, 0, initialAmount); + function testFuzz_decreaseAllowance(address account_, uint256 initialAmount_, uint256 subtractedAmount_) external { + initialAmount_ = constrictToRange(initialAmount_, 0, type(uint256).max); + subtractedAmount_ = constrictToRange(subtractedAmount_, 0, initialAmount_); - token.approve(account, initialAmount); + _token.approve(account_, initialAmount_); - assertEq(token.allowance(self, account), initialAmount); + assertEq(_token.allowance(self, account_), initialAmount_); - assertTrue(token.decreaseAllowance(account, subtractedAmount)); + assertTrue(_token.decreaseAllowance(account_, subtractedAmount_)); - assertEq(token.allowance(self, account), initialAmount - subtractedAmount); + assertEq(_token.allowance(self, account_), initialAmount_ - subtractedAmount_); } - function test_transfer(address account, uint256 amount) public { - token.mint(self, amount); + function testFuzz_transfer(address account_, uint256 amount_) external { + _token.mint(self, amount_); - assertTrue(token.transfer(account, amount)); + assertTrue(_token.transfer(account_, amount_)); - assertEq(token.totalSupply(), amount); + assertEq(_token.totalSupply(), amount_); - if (self == account) { - assertEq(token.balanceOf(self), amount); + if (self == account_) { + assertEq(_token.balanceOf(self), amount_); } else { - assertEq(token.balanceOf(self), 0); - assertEq(token.balanceOf(account), amount); + assertEq(_token.balanceOf(self), 0); + assertEq(_token.balanceOf(account_), amount_); } } - function test_transferFrom(address to, uint256 approval, uint256 amount) public { - if (amount > approval) return; // Owner must approve for more than amount. + function testFuzz_transferFrom(address recipient_, uint256 approval_, uint256 amount_) external { + if (amount_ > approval_) return; // Owner must approve for more than amount. ERC20User owner = new ERC20User(); - token.mint(address(owner), amount); - owner.erc20_approve(address(token), self, approval); + _token.mint(address(owner), amount_); + owner.erc20_approve(address(_token), self, approval_); - assertTrue(token.transferFrom(address(owner), to, amount)); + assertTrue(_token.transferFrom(address(owner), recipient_, amount_)); - assertEq(token.totalSupply(), amount); + assertEq(_token.totalSupply(), amount_); - approval = address(owner) == self ? approval : approval - amount; + approval_ = address(owner) == self ? approval_ : approval_ - amount_; - assertEq(token.allowance(address(owner), self), approval); + assertEq(_token.allowance(address(owner), self), approval_); - if (address(owner) == to) { - assertEq(token.balanceOf(address(owner)), amount); + if (address(owner) == recipient_) { + assertEq(_token.balanceOf(address(owner)), amount_); } else { - assertEq(token.balanceOf(address(owner)), 0); - assertEq(token.balanceOf(to), amount); + assertEq(_token.balanceOf(address(owner)), 0); + assertEq(_token.balanceOf(recipient_), amount_); } } - function test_transfer_insufficientBalance(address to, uint256 amount) public { - amount = amount == 0 ? 1 : amount; + function testFuzz_transfer_insufficientBalance(address recipient_, uint256 amount_) external { + amount_ = amount_ == 0 ? 1 : amount_; ERC20User account = new ERC20User(); - token.mint(address(account), amount - 1); + _token.mint(address(account), amount_ - 1); vm.expectRevert(ARITHMETIC_ERROR); - account.erc20_transfer(address(token), to, amount); + account.erc20_transfer(address(_token), recipient_, amount_); - token.mint(address(account), 1); - account.erc20_transfer(address(token), to, amount); + _token.mint(address(account), 1); + account.erc20_transfer(address(_token), recipient_, amount_); - assertEq(token.balanceOf(to), amount); + assertEq(_token.balanceOf(recipient_), amount_); } - function test_transferFrom_insufficientAllowance(address to, uint256 amount) public { - amount = amount == 0 ? 1 : amount; + function testFuzz_transferFrom_insufficientAllowance(address recipient_, uint256 amount_) external { + amount_ = amount_ == 0 ? 1 : amount_; ERC20User owner = new ERC20User(); - token.mint(address(owner), amount); + _token.mint(address(owner), amount_); - owner.erc20_approve(address(token), self, amount - 1); + owner.erc20_approve(address(_token), self, amount_ - 1); vm.expectRevert(ARITHMETIC_ERROR); - token.transferFrom(address(owner), to, amount); + _token.transferFrom(address(owner), recipient_, amount_); - owner.erc20_approve(address(token), self, amount); - token.transferFrom(address(owner), to, amount); + owner.erc20_approve(address(_token), self, amount_); + _token.transferFrom(address(owner), recipient_, amount_); - assertEq(token.balanceOf(to), amount); + assertEq(_token.balanceOf(recipient_), amount_); } - function test_transferFrom_insufficientBalance(address to, uint256 amount) public { - amount = amount == 0 ? 1 : amount; + function testFuzz_transferFrom_insufficientBalance(address recipient_, uint256 amount_) external { + amount_ = amount_ == 0 ? 1 : amount_; ERC20User owner = new ERC20User(); - token.mint(address(owner), amount - 1); - owner.erc20_approve(address(token), self, amount); + _token.mint(address(owner), amount_ - 1); + owner.erc20_approve(address(_token), self, amount_); vm.expectRevert(ARITHMETIC_ERROR); - token.transferFrom(address(owner), to, amount); + _token.transferFrom(address(owner), recipient_, amount_); - token.mint(address(owner), 1); - token.transferFrom(address(owner), to, amount); + _token.mint(address(owner), 1); + _token.transferFrom(address(owner), recipient_, amount_); - assertEq(token.balanceOf(to), amount); + assertEq(_token.balanceOf(recipient_), amount_); } } contract ERC20PermitTest is TestUtils { - bytes constant ARITHMETIC_ERROR = abi.encodeWithSignature("Panic(uint256)", 0x11); + bytes internal constant ARITHMETIC_ERROR = abi.encodeWithSignature("Panic(uint256)", 0x11); - uint256 constant S_VALUE_INCLUSIVE_UPPER_BOUND = 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0; + uint256 internal constant S_VALUE_INCLUSIVE_UPPER_BOUND = uint256(0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0); + uint256 internal constant WAD = 10 ** 18; - ERC20 token; - ERC20User user; + address internal _owner; + address internal _spender; - uint256 skOwner = 1; - uint256 skSpender = 2; - uint256 nonce = 0; - uint256 deadline = 5_000_000_000; // Timestamp far in the future + uint256 internal _skOwner = 1; + uint256 internal _skSpender = 2; + uint256 internal _nonce = 0; + uint256 internal _deadline = 5_000_000_000; // Timestamp far in the future - address owner; - address spender; + ERC20 internal _token; + ERC20User internal _user; - uint256 constant WAD = 10 ** 18; + function setUp() external virtual { + _owner = vm.addr(_skOwner); + _spender = vm.addr(_skSpender); - function setUp() public virtual { - owner = vm.addr(skOwner); - spender = vm.addr(skSpender); + vm.warp(_deadline - 52 weeks); - vm.warp(deadline - 52 weeks); - token = new ERC20("Maple Token", "MPL", 18); - user = new ERC20User(); + _token = new ERC20("Maple Token", "MPL", 18); + _user = new ERC20User(); } function test_typehash() external { - assertEq(token.PERMIT_TYPEHASH(), keccak256("Permit(address owner,address spender,uint256 amount,uint256 nonce,uint256 deadline)")); + assertEq(_token.PERMIT_TYPEHASH(), keccak256("Permit(address owner,address spender,uint256 amount,uint256 nonce,uint256 deadline)")); } // NOTE: Virtual so inheriting tests can override with different DOMAIN_SEPARATORs because of different addresses function test_domainSeparator() external virtual { - assertEq(token.DOMAIN_SEPARATOR(), 0x06c0ee43424d25534e5af6b6af862333b542f6583ff9948b8299442926099eec); + assertEq(_token.DOMAIN_SEPARATOR(), 0x06c0ee43424d25534e5af6b6af862333b542f6583ff9948b8299442926099eec); } - function test_permit() external { - uint256 amount = 10 * WAD; - assertEq(token.nonces(owner), 0); - assertEq(token.allowance(owner, spender), 0); + function test_initialState() external { + assertEq(_token.nonces(_owner), 0); + assertEq(_token.allowance(_owner, _spender), 0); + } + + function testFuzz_permit(uint256 amount_) public { + uint256 startingNonce = _token.nonces(_owner); + uint256 expectedNonce = startingNonce + 1; + + ( uint8 v, bytes32 r, bytes32 s ) = _getValidPermitSignature(address(_token), _owner, _spender, amount_, startingNonce, _deadline, _skOwner); + + _user.erc20_permit(address(_token), _owner, _spender, amount_, _deadline, v, r, s); - ( uint8 v, bytes32 r, bytes32 s ) = _getValidPermitSignature(amount, owner, skOwner, deadline); - user.erc20_permit(address(token), owner, spender, amount, deadline, v, r, s); + assertEq(_token.nonces(_owner), expectedNonce); + assertEq(_token.allowance(_owner, _spender), amount_); + } - assertEq(token.allowance(owner, spender), amount); - assertEq(token.nonces(owner), 1); + function testFuzz_permit_multiple(bytes32 seed_) external { + for (uint256 i; i < 10; ++i) { + testFuzz_permit(uint256(keccak256(abi.encodePacked(seed_, i)))); + } } function test_permit_zeroAddress() external { - uint256 amount = 10 * WAD; - ( uint8 v, bytes32 r, bytes32 s ) = _getValidPermitSignature(amount, owner, skOwner, deadline); + ( uint8 v, bytes32 r, bytes32 s ) = _getValidPermitSignature(address(_token), _owner, _spender, 1000, 0, _deadline, _skOwner); - vm.expectRevert(bytes("ERC20:P:INVALID_SIGNATURE")); - user.erc20_permit(address(token), address(0), spender, amount, deadline, v, r, s); + vm.expectRevert("ERC20:P:INVALID_SIGNATURE"); + _user.erc20_permit(address(_token), address(0), _spender, 1000, _deadline, v, r, s); } - function test_permit_nonOwnerAddress() external { - uint256 amount = 10 * WAD; + function test_permit_differentSpender() external { + ( uint8 v, bytes32 r, bytes32 s ) = _getValidPermitSignature(address(_token), _owner, address(1111), 1000, 0, _deadline, _skOwner); - ( uint8 v, bytes32 r, bytes32 s ) = _getValidPermitSignature(amount, owner, skOwner, deadline); - - vm.expectRevert(bytes("ERC20:P:INVALID_SIGNATURE")); - user.erc20_permit(address(token), spender, owner, amount, deadline, v, r, s); + // Using permit with unintended spender should fail. + vm.expectRevert("ERC20:P:INVALID_SIGNATURE"); + _user.erc20_permit(address(_token), _owner, _spender, 1000, _deadline, v, r, s); + } - ( v, r, s ) = _getValidPermitSignature(amount, spender, skSpender, deadline); + function test_permit_ownerSignerMismatch() external { + ( uint8 v, bytes32 r, bytes32 s ) = _getValidPermitSignature(address(_token), _owner, _spender, 1000, 0, _deadline, _skSpender); - vm.expectRevert(bytes("ERC20:P:INVALID_SIGNATURE")); - user.erc20_permit(address(token), owner, spender, amount, deadline, v, r, s); + vm.expectRevert("ERC20:P:INVALID_SIGNATURE"); + _user.erc20_permit(address(_token), _owner, _spender, 1000, _deadline, v, r, s); } function test_permit_withExpiry() external { - uint256 amount = 10 * WAD; uint256 expiry = 482112000 + 1 hours; // Expired permit should fail vm.warp(482112000 + 1 hours + 1); + assertEq(block.timestamp, 482112000 + 1 hours + 1); - ( uint8 v, bytes32 r, bytes32 s ) = _getValidPermitSignature(amount, owner, skOwner, expiry); + ( uint8 v, bytes32 r, bytes32 s ) = _getValidPermitSignature(address(_token), _owner, _spender, 1000, 0, expiry, _skOwner); - vm.expectRevert(bytes("ERC20:P:EXPIRED")); - user.erc20_permit(address(token), owner, spender, amount, expiry, v, r, s); + vm.expectRevert("ERC20:P:EXPIRED"); + _user.erc20_permit(address(_token), _owner, _spender, 1000, expiry, v, r, s); - assertEq(token.allowance(owner, spender), 0); - assertEq(token.nonces(owner), 0); + assertEq(_token.allowance(_owner, _spender), 0); + assertEq(_token.nonces(_owner), 0); // Valid permit should succeed vm.warp(482112000 + 1 hours); + assertEq(block.timestamp, 482112000 + 1 hours); - ( v, r, s ) = _getValidPermitSignature(amount, owner, skOwner, expiry); - user.erc20_permit(address(token), owner, spender, amount, expiry, v, r, s); + ( v, r, s ) = _getValidPermitSignature(address(_token), _owner, _spender, 1000, 0, expiry, _skOwner); + + _user.erc20_permit(address(_token), _owner, _spender, 1000, expiry, v, r, s); - assertEq(token.allowance(owner, spender), amount); - assertEq(token.nonces(owner), 1); + assertEq(_token.allowance(_owner, _spender), 1000); + assertEq(_token.nonces(_owner), 1); } function test_permit_replay() external { - uint256 amount = 10 * WAD; - ( uint8 v, bytes32 r, bytes32 s ) = _getValidPermitSignature(amount, owner, skOwner, deadline); + ( uint8 v, bytes32 r, bytes32 s ) = _getValidPermitSignature(address(_token), _owner, _spender, 1000, 0, _deadline, _skOwner); // First time should succeed - user.erc20_permit(address(token), owner, spender, amount, deadline, v, r, s); + _user.erc20_permit(address(_token), _owner, _spender, 1000, _deadline, v, r, s); // Second time nonce has been consumed and should fail - vm.expectRevert(bytes("ERC20:P:INVALID_SIGNATURE")); - user.erc20_permit(address(token), owner, spender, amount, deadline, v, r, s); + vm.expectRevert("ERC20:P:INVALID_SIGNATURE"); + _user.erc20_permit(address(_token), _owner, _spender, 1000, _deadline, v, r, s); + } + + function test_permit_earlyNonce() external { + ( uint8 v, bytes32 r, bytes32 s ) = _getValidPermitSignature(address(_token), _owner, _spender, 1000, 1, _deadline, _skOwner); + + // Previous nonce of 0 has not been consumed yet, so nonce of 1 should fail. + vm.expectRevert("ERC20:P:INVALID_SIGNATURE"); + _user.erc20_permit(address(_token), _owner, _spender, 1000, _deadline, v, r, s); + } + + function test_permit_differentVerifier() external { + address someToken = address(new ERC20("Some Token", "ST", 18)); + + ( uint8 v, bytes32 r, bytes32 s ) = _getValidPermitSignature(someToken, _owner, _spender, 1000, 0, _deadline, _skOwner); + + // Using permit with unintended verifier should fail. + vm.expectRevert("ERC20:P:INVALID_SIGNATURE"); + _user.erc20_permit(address(_token), _owner, _spender, 1000, _deadline, v, r, s); } function test_permit_badS() external { - uint256 amount = 10 * WAD; - ( uint8 v, bytes32 r, bytes32 s ) = _getValidPermitSignature(amount, owner, skOwner, deadline); + ( uint8 v, bytes32 r, ) = _getValidPermitSignature(address(_token), _owner, _spender, 1000, 0, _deadline, _skOwner); // Send in an s that is above the upper bound. bytes32 badS = bytes32(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); + vm.expectRevert("ERC20:P:MALLEABLE"); + _user.erc20_permit(address(_token), _owner, _spender, 1000, _deadline, v, r, badS); } function test_permit_badV() 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); + ( uint8 v, bytes32 r, bytes32 s ) = _getValidPermitSignature(address(_token), _owner, _spender, 1000, 0, _deadline, _skOwner); for (uint8 i; i <= type(uint8).max; i++) { if (i == type(uint8).max) { break; } else if (i != 27 && i != 28) { - vm.expectRevert(bytes("ERC20:P:MALLEABLE")); + vm.expectRevert("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")); - } + if (i == v) continue; + + // Should get past the Malleable require check as 27 or 28 are valid values for s. + vm.expectRevert("ERC20:P:INVALID_SIGNATURE"); } - user.erc20_permit(address(token), owner, spender, amount, deadline, i, r, s); - } - user.erc20_permit(address(token), owner, spender, amount, deadline, v, r, s); + _user.erc20_permit(address(_token), _owner, _spender, 1000, _deadline, i, 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) { + function _getDigest(address token_, address owner_, address spender_, uint256 amount_, uint256 nonce_, uint256 deadline_) internal view returns (bytes32 digest_) { return keccak256( abi.encodePacked( '\x19\x01', - token.DOMAIN_SEPARATOR(), - keccak256(abi.encode(token.PERMIT_TYPEHASH(), owner_, spender_, value_, nonce_, deadline_)) + IERC20(token_).DOMAIN_SEPARATOR(), + keccak256(abi.encode(IERC20(token_).PERMIT_TYPEHASH(), owner_, spender_, amount_, nonce_, deadline_)) ) ); } // Returns a valid `permit` signature signed by this contract's `owner` address - function _getValidPermitSignature(uint256 value_, address owner_, uint256 ownerSk_, uint256 deadline_) internal returns (uint8 v_, bytes32 r_, bytes32 s_) { - bytes32 digest = _getDigest(owner_, spender, value_, nonce, deadline_); - ( uint8 v, bytes32 r, bytes32 s ) = vm.sign(ownerSk_, digest); - return (v, r, s); + function _getValidPermitSignature(address token_, address owner_, address spender_, uint256 amount_, uint256 nonce_, uint256 deadline_, uint256 ownerSk_) internal returns (uint8 v_, bytes32 r_, bytes32 s_) { + return vm.sign(ownerSk_, _getDigest(token_, owner_, spender_, amount_, nonce_, deadline_)); } } contract ERC20Invariants is TestUtils, InvariantTest { - BalanceSum balanceSum; + BalanceSum internal _balanceSum; + + function setUp() external { + _balanceSum = new BalanceSum(); - function setUp() public { - balanceSum = new BalanceSum(); - addTargetContract(address(balanceSum)); + addTargetContract(address(_balanceSum)); } - function invariant_balanceSum() public { - assertEq(balanceSum.token().totalSupply(), balanceSum.sum()); + function invariant_balanceSum() external { + assertEq(_balanceSum.token().totalSupply(), _balanceSum.sum()); } } @@ -365,26 +388,26 @@ contract BalanceSum { uint256 public sum; - function mint(address account, uint256 amount) external { - token.mint(account, amount); - sum += amount; + function mint(address recipient_, uint256 amount_) external { + token.mint(recipient_, amount_); + sum += amount_; } - function burn(address account, uint256 amount) external { - token.burn(account, amount); - sum -= amount; + function burn(address owner_, uint256 amount_) external { + token.burn(owner_, amount_); + sum -= amount_; } - function approve(address dst, uint256 amount) external { - token.approve(dst, amount); + function approve(address spender_, uint256 amount_) external { + token.approve(spender_, amount_); } - function transferFrom(address src, address dst, uint256 amount) external { - token.transferFrom(src, dst, amount); + function transferFrom(address owner_, address recipient_, uint256 amount_) external { + token.transferFrom(owner_, recipient_, amount_); } - function transfer(address dst, uint256 amount) external { - token.transfer(dst, amount); + function transfer(address recipient_, uint256 amount_) external { + token.transfer(recipient_, amount_); } } diff --git a/contracts/test/accounts/ERC20User.sol b/contracts/test/accounts/ERC20User.sol index 150a9be..bfd55a4 100644 --- a/contracts/test/accounts/ERC20User.sol +++ b/contracts/test/accounts/ERC20User.sol @@ -24,12 +24,12 @@ contract ERC20User { IERC20(token_).permit(owner_, spender_, amount_, deadline_, v_, r_, s_); } - function erc20_transfer(address token_, address recipient_, uint256 amount_) external { - IERC20(token_).transfer(recipient_, amount_); + function erc20_transfer(address token_, address recipient_, uint256 amount_) external returns (bool success_) { + return IERC20(token_).transfer(recipient_, amount_); } - function erc20_transferFrom(address token_, address owner_, address recipient_, uint256 amount_) external { - IERC20(token_).transferFrom(owner_, recipient_, amount_); + function erc20_transferFrom(address token_, address owner_, address recipient_, uint256 amount_) external returns (bool success_) { + return IERC20(token_).transferFrom(owner_, recipient_, amount_); } } diff --git a/contracts/test/mocks/MockERC20.sol b/contracts/test/mocks/MockERC20.sol index faee207..b1a0c80 100644 --- a/contracts/test/mocks/MockERC20.sol +++ b/contracts/test/mocks/MockERC20.sol @@ -7,12 +7,12 @@ contract MockERC20 is ERC20 { constructor(string memory name_, string memory symbol_, uint8 decimals_) ERC20(name_, symbol_, decimals_) { } - function mint(address to_, uint256 value_) external { - _mint(to_, value_); + function mint(address recipient_, uint256 amount_) external { + _mint(recipient_, amount_); } - function burn(address from_, uint256 value_) external { - _burn(from_, value_); + function burn(address owner_, uint256 amount_) external { + _burn(owner_, amount_); } }