From e8334bdd55c7eb93be3e036ab134ab7f42154ece Mon Sep 17 00:00:00 2001 From: t11s Date: Sun, 23 Oct 2022 22:36:57 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=A8=20Style=20nits=20(#51)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ♻️ Style * ⚡️ Gasolina Co-authored-by: Sara Reynolds --- .forge-snapshots/permitBatchCleanWrite.snap | 2 +- .forge-snapshots/permitBatchDirtyWrite.snap | 2 +- ...permitBatchTransferFromMultipleTokens.snap | 2 +- .../permitBatchTransferFromSingleToken.snap | 2 +- .forge-snapshots/permitCleanWrite.snap | 2 +- .forge-snapshots/permitDirtyNonce.snap | 2 +- .forge-snapshots/permitDirtyWrite.snap | 2 +- .forge-snapshots/permitInvalidSigner.snap | 2 +- .../permitSetMaxAllowanceCleanWrite.snap | 2 +- .../permitSetMaxAllowanceDirtyWrite.snap | 2 +- .../permitTransferFromSingleToken.snap | 2 +- .../single recipient 2 tokens.snap | 2 +- .../single recipient many tokens.snap | 2 +- .gas-snapshot | 80 +++++++++---------- src/AllowanceTransfer.sol | 23 +----- src/EIP712.sol | 25 +++--- src/SignatureTransfer.sol | 8 +- 17 files changed, 71 insertions(+), 91 deletions(-) diff --git a/.forge-snapshots/permitBatchCleanWrite.snap b/.forge-snapshots/permitBatchCleanWrite.snap index 3e66135..ea953ea 100644 --- a/.forge-snapshots/permitBatchCleanWrite.snap +++ b/.forge-snapshots/permitBatchCleanWrite.snap @@ -1 +1 @@ -89564 \ No newline at end of file +89516 \ No newline at end of file diff --git a/.forge-snapshots/permitBatchDirtyWrite.snap b/.forge-snapshots/permitBatchDirtyWrite.snap index 3a257c5..aa4b093 100644 --- a/.forge-snapshots/permitBatchDirtyWrite.snap +++ b/.forge-snapshots/permitBatchDirtyWrite.snap @@ -1 +1 @@ -55364 \ No newline at end of file +55316 \ No newline at end of file diff --git a/.forge-snapshots/permitBatchTransferFromMultipleTokens.snap b/.forge-snapshots/permitBatchTransferFromMultipleTokens.snap index f4d1906..2c46d20 100644 --- a/.forge-snapshots/permitBatchTransferFromMultipleTokens.snap +++ b/.forge-snapshots/permitBatchTransferFromMultipleTokens.snap @@ -1 +1 @@ -145155 \ No newline at end of file +145107 \ No newline at end of file diff --git a/.forge-snapshots/permitBatchTransferFromSingleToken.snap b/.forge-snapshots/permitBatchTransferFromSingleToken.snap index a3af195..c24be8a 100644 --- a/.forge-snapshots/permitBatchTransferFromSingleToken.snap +++ b/.forge-snapshots/permitBatchTransferFromSingleToken.snap @@ -1 +1 @@ -90375 \ No newline at end of file +90327 \ No newline at end of file diff --git a/.forge-snapshots/permitCleanWrite.snap b/.forge-snapshots/permitCleanWrite.snap index de203e1..18ec613 100644 --- a/.forge-snapshots/permitCleanWrite.snap +++ b/.forge-snapshots/permitCleanWrite.snap @@ -1 +1 @@ -61245 \ No newline at end of file +61197 \ No newline at end of file diff --git a/.forge-snapshots/permitDirtyNonce.snap b/.forge-snapshots/permitDirtyNonce.snap index b4ca734..fcd7d5b 100644 --- a/.forge-snapshots/permitDirtyNonce.snap +++ b/.forge-snapshots/permitDirtyNonce.snap @@ -1 +1 @@ -42146 \ No newline at end of file +42098 \ No newline at end of file diff --git a/.forge-snapshots/permitDirtyWrite.snap b/.forge-snapshots/permitDirtyWrite.snap index e7a89f7..c8ad870 100644 --- a/.forge-snapshots/permitDirtyWrite.snap +++ b/.forge-snapshots/permitDirtyWrite.snap @@ -1 +1 @@ -44145 \ No newline at end of file +44097 \ No newline at end of file diff --git a/.forge-snapshots/permitInvalidSigner.snap b/.forge-snapshots/permitInvalidSigner.snap index e1eeec4..5904007 100644 --- a/.forge-snapshots/permitInvalidSigner.snap +++ b/.forge-snapshots/permitInvalidSigner.snap @@ -1 +1 @@ -42875 \ No newline at end of file +42827 \ No newline at end of file diff --git a/.forge-snapshots/permitSetMaxAllowanceCleanWrite.snap b/.forge-snapshots/permitSetMaxAllowanceCleanWrite.snap index e9d41ba..05d894b 100644 --- a/.forge-snapshots/permitSetMaxAllowanceCleanWrite.snap +++ b/.forge-snapshots/permitSetMaxAllowanceCleanWrite.snap @@ -1 +1 @@ -59246 \ No newline at end of file +59198 \ No newline at end of file diff --git a/.forge-snapshots/permitSetMaxAllowanceDirtyWrite.snap b/.forge-snapshots/permitSetMaxAllowanceDirtyWrite.snap index b4ca734..fcd7d5b 100644 --- a/.forge-snapshots/permitSetMaxAllowanceDirtyWrite.snap +++ b/.forge-snapshots/permitSetMaxAllowanceDirtyWrite.snap @@ -1 +1 @@ -42146 \ No newline at end of file +42098 \ No newline at end of file diff --git a/.forge-snapshots/permitTransferFromSingleToken.snap b/.forge-snapshots/permitTransferFromSingleToken.snap index 3c580a8..d6dbd4b 100644 --- a/.forge-snapshots/permitTransferFromSingleToken.snap +++ b/.forge-snapshots/permitTransferFromSingleToken.snap @@ -1 +1 @@ -88572 \ No newline at end of file +88524 \ No newline at end of file diff --git a/.forge-snapshots/single recipient 2 tokens.snap b/.forge-snapshots/single recipient 2 tokens.snap index 2a3f84f..c394a02 100644 --- a/.forge-snapshots/single recipient 2 tokens.snap +++ b/.forge-snapshots/single recipient 2 tokens.snap @@ -1 +1 @@ -120178 \ No newline at end of file +120130 \ No newline at end of file diff --git a/.forge-snapshots/single recipient many tokens.snap b/.forge-snapshots/single recipient many tokens.snap index 470ceed..eb2c874 100644 --- a/.forge-snapshots/single recipient many tokens.snap +++ b/.forge-snapshots/single recipient many tokens.snap @@ -1 +1 @@ -136103 \ No newline at end of file +136055 \ No newline at end of file diff --git a/.gas-snapshot b/.gas-snapshot index a3208e9..3e11a3d 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -1,53 +1,53 @@ AllowanceTransferTest:testApprove() (gas: 42144) -AllowanceTransferTest:testBatchTransferFrom() (gas: 156043) -AllowanceTransferTest:testBatchTransferFromLengthMismatch() (gas: 75628) -AllowanceTransferTest:testBatchTransferFromWithGasSnapshot() (gas: 156439) -AllowanceTransferTest:testExcessiveInvalidation() (gas: 59214) +AllowanceTransferTest:testBatchTransferFrom() (gas: 155995) +AllowanceTransferTest:testBatchTransferFromLengthMismatch() (gas: 75580) +AllowanceTransferTest:testBatchTransferFromWithGasSnapshot() (gas: 156391) +AllowanceTransferTest:testExcessiveInvalidation() (gas: 59166) AllowanceTransferTest:testInvalidateNonces() (gas: 56453) -AllowanceTransferTest:testMaxAllowance() (gas: 132582) -AllowanceTransferTest:testMaxAllowanceDirtyWrite() (gas: 115306) -AllowanceTransferTest:testPartialAllowance() (gas: 102786) -AllowanceTransferTest:testReuseOrderedNonceInvalid() (gas: 62020) -AllowanceTransferTest:testSetAllowance() (gas: 87131) -AllowanceTransferTest:testSetAllowanceBatch() (gas: 129988) -AllowanceTransferTest:testSetAllowanceBatchDirtyWrite() (gas: 95549) +AllowanceTransferTest:testMaxAllowance() (gas: 132534) +AllowanceTransferTest:testMaxAllowanceDirtyWrite() (gas: 115258) +AllowanceTransferTest:testPartialAllowance() (gas: 102738) +AllowanceTransferTest:testReuseOrderedNonceInvalid() (gas: 61972) +AllowanceTransferTest:testSetAllowance() (gas: 87083) +AllowanceTransferTest:testSetAllowanceBatch() (gas: 129940) +AllowanceTransferTest:testSetAllowanceBatchDirtyWrite() (gas: 95501) AllowanceTransferTest:testSetAllowanceDeadlinePassed() (gas: 57607) -AllowanceTransferTest:testSetAllowanceDirtyWrite() (gas: 69789) -AllowanceTransferTest:testSetAllowanceInvalidSignature() (gas: 66207) -AllowanceTransferTest:testSetAllowanceTransfer() (gas: 100965) -AllowanceTransferTest:testSetAllowanceTransferDirtyNonceDirtyTransfer() (gas: 95235) -AllowanceTransferTest:testTransferFromWithGasSnapshot() (gas: 130852) -EIP712Test:testDomainSeparator() (gas: 5785) -EIP712Test:testDomainSeparatorAfterFork() (gas: 10730) +AllowanceTransferTest:testSetAllowanceDirtyWrite() (gas: 69741) +AllowanceTransferTest:testSetAllowanceInvalidSignature() (gas: 66159) +AllowanceTransferTest:testSetAllowanceTransfer() (gas: 100917) +AllowanceTransferTest:testSetAllowanceTransferDirtyNonceDirtyTransfer() (gas: 95187) +AllowanceTransferTest:testTransferFromWithGasSnapshot() (gas: 130804) +EIP712Test:testDomainSeparator() (gas: 5737) +EIP712Test:testDomainSeparatorAfterFork() (gas: 10586) NonceBitmapTest:testHighNonces() (gas: 36214) NonceBitmapTest:testInvalidateFullWord() (gas: 62732) NonceBitmapTest:testInvalidateNonzeroWord() (gas: 85931) NonceBitmapTest:testLowNonces() (gas: 41250) NonceBitmapTest:testNonceWordBoundary() (gas: 58074) -NonceBitmapTest:testUseTwoRandomNonces(uint256,uint256) (runs: 256, μ: 49388, ~: 51913) +NonceBitmapTest:testUseTwoRandomNonces(uint256,uint256) (runs: 256, μ: 49473, ~: 51913) NonceBitmapTest:testUsingNonceTwiceFails(uint256) (runs: 256, μ: 32679, ~: 32679) -SignatureTransferTest:testGasMultiplePermitBatchTransferFrom() (gas: 266625) -SignatureTransferTest:testGasSinglePermitBatchTransferFrom() (gas: 183516) -SignatureTransferTest:testGasSinglePermitTransferFrom() (gas: 123494) +SignatureTransferTest:testGasMultiplePermitBatchTransferFrom() (gas: 266577) +SignatureTransferTest:testGasSinglePermitBatchTransferFrom() (gas: 183468) +SignatureTransferTest:testGasSinglePermitTransferFrom() (gas: 123446) SignatureTransferTest:testInvalidateUnorderedNonces() (gas: 55784) -SignatureTransferTest:testPermitBatchTransferFrom() (gas: 159502) -SignatureTransferTest:testPermitBatchTransferFromSingleRecipient() (gas: 186009) -SignatureTransferTest:testPermitBatchTransferFromTypedWitness() (gas: 163580) -SignatureTransferTest:testPermitBatchTransferFromTypedWitnessInvalidType() (gas: 82819) -SignatureTransferTest:testPermitBatchTransferFromTypedWitnessInvalidTypeHash() (gas: 82530) -SignatureTransferTest:testPermitBatchTransferFromTypedWitnessInvalidTypeName() (gas: 83698) -SignatureTransferTest:testPermitBatchTransferFromTypedWitnessInvalidWitness() (gas: 83731) +SignatureTransferTest:testPermitBatchTransferFrom() (gas: 159454) +SignatureTransferTest:testPermitBatchTransferFromSingleRecipient() (gas: 185961) +SignatureTransferTest:testPermitBatchTransferFromTypedWitness() (gas: 163532) +SignatureTransferTest:testPermitBatchTransferFromTypedWitnessInvalidType() (gas: 82771) +SignatureTransferTest:testPermitBatchTransferFromTypedWitnessInvalidTypeHash() (gas: 82482) +SignatureTransferTest:testPermitBatchTransferFromTypedWitnessInvalidTypeName() (gas: 83650) +SignatureTransferTest:testPermitBatchTransferFromTypedWitnessInvalidWitness() (gas: 83683) SignatureTransferTest:testPermitBatchTransferInvalidAmountsLength() (gas: 43943) SignatureTransferTest:testPermitBatchTransferInvalidRecipientsLength() (gas: 43210) SignatureTransferTest:testPermitBatchTransferInvalidSingleAddr() (gas: 41912) -SignatureTransferTest:testPermitBatchTransferMultiAddr() (gas: 157816) -SignatureTransferTest:testPermitBatchTransferSingleRecipientManyTokens() (gas: 195744) -SignatureTransferTest:testPermitTransferFrom() (gas: 92654) -SignatureTransferTest:testPermitTransferFromInvalidNonce() (gas: 92270) -SignatureTransferTest:testPermitTransferFromRandomNonceAndAmount(uint256,uint128) (runs: 256, μ: 95241, ~: 96217) -SignatureTransferTest:testPermitTransferFromToSpender() (gas: 97019) -SignatureTransferTest:testPermitTransferFromTypedWitness() (gas: 96460) -SignatureTransferTest:testPermitTransferFromTypedWitnessInvalidType() (gas: 57391) -SignatureTransferTest:testPermitTransferFromTypedWitnessInvalidTypeName() (gas: 58358) -SignatureTransferTest:testPermitTransferFromTypedWitnessInvalidTypehash() (gas: 56738) -SignatureTransferTest:testPermitTransferSpendLessThanFull(uint256,uint128) (runs: 256, μ: 97554, ~: 99272) +SignatureTransferTest:testPermitBatchTransferMultiAddr() (gas: 157768) +SignatureTransferTest:testPermitBatchTransferSingleRecipientManyTokens() (gas: 195696) +SignatureTransferTest:testPermitTransferFrom() (gas: 92606) +SignatureTransferTest:testPermitTransferFromInvalidNonce() (gas: 92222) +SignatureTransferTest:testPermitTransferFromRandomNonceAndAmount(uint256,uint128) (runs: 256, μ: 95193, ~: 96169) +SignatureTransferTest:testPermitTransferFromToSpender() (gas: 96971) +SignatureTransferTest:testPermitTransferFromTypedWitness() (gas: 96412) +SignatureTransferTest:testPermitTransferFromTypedWitnessInvalidType() (gas: 57343) +SignatureTransferTest:testPermitTransferFromTypedWitnessInvalidTypeName() (gas: 58310) +SignatureTransferTest:testPermitTransferFromTypedWitnessInvalidTypehash() (gas: 56690) +SignatureTransferTest:testPermitTransferSpendLessThanFull(uint256,uint128) (runs: 256, μ: 97506, ~: 99224) diff --git a/src/AllowanceTransfer.sol b/src/AllowanceTransfer.sol index cdb2b1c..9333368 100644 --- a/src/AllowanceTransfer.sol +++ b/src/AllowanceTransfer.sol @@ -17,10 +17,6 @@ contract AllowanceTransfer is IAllowanceTransfer, EIP712 { event InvalidateNonces(address indexed owner, uint32 indexed toNonce, address token, address spender); - /*////////////////////////////////////////////////////////////// - ALLOWANCE STORAGE - //////////////////////////////////////////////////////////////*/ - /// @notice Maps users to tokens to spender addresses and information about the approval on the token /// @dev Indexed in the order of token owner address, token address, spender address /// @dev The stored word saves the allowed amount, expiration on the allowance, and nonce @@ -33,10 +29,6 @@ contract AllowanceTransfer is IAllowanceTransfer, EIP712 { allowed.expiration = expiration; } - /*/////////////////////////////////////////////////////f///////// - PERMIT LOGIC - //////////////////////////////////////////////////////////////*/ - /// @inheritdoc IAllowanceTransfer function permit(Permit calldata permitData, address owner, bytes calldata signature) external { PackedAllowance storage allowed = allowance[owner][permitData.token][permitData.spender]; @@ -89,10 +81,6 @@ contract AllowanceTransfer is IAllowanceTransfer, EIP712 { if (nonce != signedNonce) revert InvalidNonce(); } - /*////////////////////////////////////////////////////////////// - TRANSFER LOGIC - //////////////////////////////////////////////////////////////*/ - /// @inheritdoc IAllowanceTransfer function transferFrom(address token, address from, address to, uint160 amount) external { _transfer(token, from, to, amount); @@ -119,11 +107,9 @@ contract AllowanceTransfer is IAllowanceTransfer, EIP712 { function _transfer(address token, address from, address to, uint160 amount) private { PackedAllowance storage allowed = allowance[from][token][msg.sender]; - if (block.timestamp > allowed.expiration) { - revert AllowanceExpired(); - } + if (block.timestamp > allowed.expiration) revert AllowanceExpired(); - uint160 maxAmount = allowed.amount; + uint256 maxAmount = allowed.amount; if (maxAmount != type(uint160).max) { if (amount > maxAmount) { revert InsufficientAllowance(); @@ -133,14 +119,11 @@ contract AllowanceTransfer is IAllowanceTransfer, EIP712 { } } } + // Transfer the tokens from the from address to the recipient. ERC20(token).safeTransferFrom(from, to, amount); } - /*////////////////////////////////////////////////////////////// - LOCKDOWN LOGIC - //////////////////////////////////////////////////////////////*/ - /// @inheritdoc IAllowanceTransfer function lockdown(address[] calldata tokens, address[] calldata spenders) external { // Each index should correspond to an index in the other array. diff --git a/src/EIP712.sol b/src/EIP712.sol index 00ccba3..bbd77f0 100644 --- a/src/EIP712.sol +++ b/src/EIP712.sol @@ -2,15 +2,13 @@ pragma solidity 0.8.17; /// @notice EIP712 helpers for permit2 -/// @dev maintains cross-chain replay protection in the event of a fork -/// @dev reference: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/EIP712.sol +/// @dev Maintains cross-chain replay protection in the event of a fork +/// @dev Reference: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/EIP712.sol contract EIP712 { - /* solhint-disable var-name-mixedcase */ - // Cache the domain separator as an immutable value, but also store the chain id that it corresponds to, in order to - // invalidate the cached domain separator if the chain id changes. + // Cache the domain separator as an immutable value, but also store the chain id that it + // corresponds to, in order to invalidate the cached domain separator if the chain id changes. bytes32 private immutable _CACHED_DOMAIN_SEPARATOR; uint256 private immutable _CACHED_CHAIN_ID; - address private immutable _CACHED_THIS; bytes32 private constant _HASHED_NAME = keccak256("Permit2"); bytes32 private constant _TYPE_HASH = @@ -18,21 +16,18 @@ contract EIP712 { constructor() { _CACHED_CHAIN_ID = block.chainid; - _CACHED_THIS = address(this); _CACHED_DOMAIN_SEPARATOR = _buildDomainSeparator(_TYPE_HASH, _HASHED_NAME); } - /// @notice returns the domain separator for the current chain - /// @dev uses cached version if chainid and address are unchanged from construction + /// @notice Returns the domain separator for the current chain. + /// @dev Uses cached version if chainid and address are unchanged from construction. function DOMAIN_SEPARATOR() public view returns (bytes32) { - if (address(this) == _CACHED_THIS && block.chainid == _CACHED_CHAIN_ID) { - return _CACHED_DOMAIN_SEPARATOR; - } else { - return _buildDomainSeparator(_TYPE_HASH, _HASHED_NAME); - } + return block.chainid == _CACHED_CHAIN_ID + ? _CACHED_DOMAIN_SEPARATOR + : _buildDomainSeparator(_TYPE_HASH, _HASHED_NAME); } - /// @notice builds a domain separator using the current chainId and contract address + /// @notice Builds a domain separator using the current chainId and contract address. function _buildDomainSeparator(bytes32 typeHash, bytes32 nameHash) private view returns (bytes32) { return keccak256(abi.encode(typeHash, nameHash, block.chainid, address(this))); } diff --git a/src/SignatureTransfer.sol b/src/SignatureTransfer.sol index 509fbee..12c3871 100644 --- a/src/SignatureTransfer.sol +++ b/src/SignatureTransfer.sol @@ -124,6 +124,7 @@ contract SignatureTransfer is ISignatureTransfer, EIP712 { ) internal { _validatePermit(permit.spender, permit.deadline); _validateInputLengths(permit.tokens.length, to.length, permit.signedAmounts.length, requestedAmounts.length); + unchecked { for (uint256 i = 0; i < permit.tokens.length; ++i) { if (requestedAmounts[i] > permit.signedAmounts[i]) revert InvalidAmount(); @@ -144,6 +145,7 @@ contract SignatureTransfer is ISignatureTransfer, EIP712 { /// @inheritdoc ISignatureTransfer function invalidateUnorderedNonces(uint256 wordPos, uint256 mask) external { nonceBitmap[msg.sender][wordPos] |= mask; + emit InvalidateUnorderedNonces(msg.sender, wordPos, mask); } @@ -164,9 +166,9 @@ contract SignatureTransfer is ISignatureTransfer, EIP712 { function _useUnorderedNonce(address from, uint256 nonce) internal { (uint248 wordPos, uint8 bitPos) = bitmapPositions(nonce); uint256 bitmap = nonceBitmap[from][wordPos]; - if ((bitmap >> bitPos) & 1 == 1) { - revert InvalidNonce(); - } + + if ((bitmap >> bitPos) & 1 == 1) revert InvalidNonce(); + nonceBitmap[from][wordPos] = bitmap | (1 << bitPos); }