Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃帹 Style nits #51

Merged
merged 3 commits into from Oct 24, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -1 +1 @@
145408
145360
2 changes: 1 addition & 1 deletion .forge-snapshots/permitBatchTransferFromSingleToken.snap
@@ -1 +1 @@
90616
90568
2 changes: 1 addition & 1 deletion .forge-snapshots/permitCleanWrite.snap
@@ -1 +1 @@
62486
62438
2 changes: 1 addition & 1 deletion .forge-snapshots/permitDirtyWrite.snap
@@ -1 +1 @@
45386
45338
2 changes: 1 addition & 1 deletion .forge-snapshots/permitInvalidSigner.snap
@@ -1 +1 @@
43267
43219
2 changes: 1 addition & 1 deletion .forge-snapshots/permitSetMaxAllowanceCleanWrite.snap
@@ -1 +1 @@
60481
60433
2 changes: 1 addition & 1 deletion .forge-snapshots/permitSetMaxAllowanceDirtyWrite.snap
@@ -1 +1 @@
43381
43333
2 changes: 1 addition & 1 deletion .forge-snapshots/permitTransferFromSingleToken.snap
@@ -1 +1 @@
88884
88836
2 changes: 1 addition & 1 deletion .forge-snapshots/single recipient 2 tokens.snap
@@ -1 +1 @@
120425
120377
2 changes: 1 addition & 1 deletion .forge-snapshots/single recipient many tokens.snap
@@ -1 +1 @@
136398
136350
60 changes: 30 additions & 30 deletions .gas-snapshot
@@ -1,37 +1,37 @@
AllowanceTransferTest:testApprove() (gas: 40056)
AllowanceTransferTest:testBatchTransferFrom() (gas: 124856)
AllowanceTransferTest:testBatchTransferFromLengthMismatch() (gas: 74485)
AllowanceTransferTest:testBatchTransferFromWithGasSnapshot() (gas: 155331)
AllowanceTransferTest:testExcessiveInvalidation() (gas: 58217)
AllowanceTransferTest:testBatchTransferFrom() (gas: 124808)
AllowanceTransferTest:testBatchTransferFromLengthMismatch() (gas: 74437)
AllowanceTransferTest:testBatchTransferFromWithGasSnapshot() (gas: 155283)
AllowanceTransferTest:testExcessiveInvalidation() (gas: 58169)
AllowanceTransferTest:testInvalidateNonces() (gas: 54185)
AllowanceTransferTest:testMaxAllowance() (gas: 131556)
AllowanceTransferTest:testMaxAllowanceDirtyWrite() (gas: 114294)
AllowanceTransferTest:testPartialAllowance() (gas: 101778)
AllowanceTransferTest:testReuseOrderedNonceInvalid() (gas: 61038)
AllowanceTransferTest:testSetAllowance() (gas: 85906)
AllowanceTransferTest:testMaxAllowance() (gas: 131508)
AllowanceTransferTest:testMaxAllowanceDirtyWrite() (gas: 114246)
AllowanceTransferTest:testPartialAllowance() (gas: 101730)
AllowanceTransferTest:testReuseOrderedNonceInvalid() (gas: 60990)
AllowanceTransferTest:testSetAllowance() (gas: 85858)
AllowanceTransferTest:testSetAllowanceDeadlinePassed() (gas: 53693)
AllowanceTransferTest:testSetAllowanceDirtyWrite() (gas: 68539)
AllowanceTransferTest:testSetAllowanceInvalidSignature() (gas: 64336)
AllowanceTransferTest:testSetAllowanceTransfer() (gas: 99965)
AllowanceTransferTest:testSetAllowanceTransferDirtyNonceDirtyTransfer() (gas: 66051)
AllowanceTransferTest:testTransferFromWithGasSnapshot() (gas: 129868)
DomainSeparatorTest:testDomainSeparator() (gas: 5807)
DomainSeparatorTest:testDomainSeparatorAfterFork() (gas: 10744)
SignatureTransferTest:testGasMultiplePermitBatchTransferFrom() (gas: 266835)
SignatureTransferTest:testGasSinglePermitBatchTransferFrom() (gas: 183692)
SignatureTransferTest:testGasSinglePermitTransferFrom() (gas: 123674)
AllowanceTransferTest:testSetAllowanceDirtyWrite() (gas: 68491)
AllowanceTransferTest:testSetAllowanceInvalidSignature() (gas: 64288)
AllowanceTransferTest:testSetAllowanceTransfer() (gas: 99917)
AllowanceTransferTest:testSetAllowanceTransferDirtyNonceDirtyTransfer() (gas: 66003)
AllowanceTransferTest:testTransferFromWithGasSnapshot() (gas: 129820)
DomainSeparatorTest:testDomainSeparator() (gas: 5759)
DomainSeparatorTest:testDomainSeparatorAfterFork() (gas: 10600)
SignatureTransferTest:testGasMultiplePermitBatchTransferFrom() (gas: 266787)
SignatureTransferTest:testGasSinglePermitBatchTransferFrom() (gas: 183644)
SignatureTransferTest:testGasSinglePermitTransferFrom() (gas: 123626)
SignatureTransferTest:testInvalidateUnorderedNonces() (gas: 55723)
SignatureTransferTest:testPermitBatchTransferFrom() (gas: 159739)
SignatureTransferTest:testPermitBatchTransferFromSingleRecipient() (gas: 186048)
SignatureTransferTest:testPermitBatchTransferFrom() (gas: 159691)
SignatureTransferTest:testPermitBatchTransferFromSingleRecipient() (gas: 186000)
SignatureTransferTest:testPermitBatchTransferInvalidAmountsLength() (gas: 42167)
SignatureTransferTest:testPermitBatchTransferInvalidRecipientsLength() (gas: 41632)
SignatureTransferTest:testPermitBatchTransferInvalidSingleAddr() (gas: 40312)
SignatureTransferTest:testPermitBatchTransferMultiAddr() (gas: 157899)
SignatureTransferTest:testPermitBatchTransferSingleRecipientManyTokens() (gas: 196007)
SignatureTransferTest:testPermitTransferFrom() (gas: 92966)
SignatureTransferTest:testPermitTransferFromInvalidNonce() (gas: 92455)
SignatureTransferTest:testPermitTransferFromToSpender() (gas: 97334)
SignatureTransferTest:testPermitTransferFromTypedWitness() (gas: 96716)
SignatureTransferTest:testPermitTransferFromTypedWitnessInvalidType() (gas: 57585)
SignatureTransferTest:testPermitTransferFromTypedWitnessInvalidTypeName() (gas: 58392)
SignatureTransferTest:testPermitTransferFromTypedWitnessInvalidTypehash() (gas: 56866)
SignatureTransferTest:testPermitBatchTransferMultiAddr() (gas: 157851)
SignatureTransferTest:testPermitBatchTransferSingleRecipientManyTokens() (gas: 195959)
SignatureTransferTest:testPermitTransferFrom() (gas: 92918)
SignatureTransferTest:testPermitTransferFromInvalidNonce() (gas: 92407)
SignatureTransferTest:testPermitTransferFromToSpender() (gas: 97286)
SignatureTransferTest:testPermitTransferFromTypedWitness() (gas: 96668)
SignatureTransferTest:testPermitTransferFromTypedWitnessInvalidType() (gas: 57537)
SignatureTransferTest:testPermitTransferFromTypedWitnessInvalidTypeName() (gas: 58344)
SignatureTransferTest:testPermitTransferFromTypedWitnessInvalidTypehash() (gas: 56818)
36 changes: 7 additions & 29 deletions src/AllowanceTransfer.sol
Expand Up @@ -29,10 +29,6 @@ contract AllowanceTransfer is DomainSeparator {

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 The saved packed word saves the allowed amount, expiration, and nonce.
mapping(address => mapping(address => mapping(address => PackedAllowance))) public allowance;
Expand All @@ -49,18 +45,12 @@ contract AllowanceTransfer is DomainSeparator {
allowed.expiration = expiration;
}

/*/////////////////////////////////////////////////////f/////////
PERMIT LOGIC
//////////////////////////////////////////////////////////////*/

/// @notice Permit a user to spend a given amount of another user's
/// approved amount of the given token via the owner's EIP-712 signature.
/// @dev May fail if the owner's nonce was invalidated in-flight by invalidateNonce.
function permit(Permit calldata permitData, address owner, bytes calldata signature) external {
// Ensure the signature's deadline has not already passed.
if (block.timestamp > permitData.sigDeadline) {
revert SignatureExpired();
}
if (block.timestamp > permitData.sigDeadline) revert SignatureExpired();

// Check current nonce (incremented below).
if (permitData.nonce != allowance[owner][permitData.token][permitData.spender].nonce) {
Expand Down Expand Up @@ -97,10 +87,6 @@ contract AllowanceTransfer is DomainSeparator {
PackedAllowance({amount: permitData.amount, expiration: expiration, nonce: permitData.nonce + 1});
}

/*//////////////////////////////////////////////////////////////
TRANSFER LOGIC
//////////////////////////////////////////////////////////////*/

/// @notice Transfer approved tokens from one address to another.
/// @param token The token to transfer.
/// @param from The address to transfer from.
Expand All @@ -115,9 +101,8 @@ contract AllowanceTransfer is DomainSeparator {
function batchTransferFrom(address[] calldata token, address from, address[] calldata to, uint160[] calldata amount)
external
{
if (amount.length != to.length || token.length != to.length) {
revert LengthMismatch();
}
if (amount.length != to.length || token.length != to.length) revert LengthMismatch();

unchecked {
for (uint256 i = 0; i < token.length; ++i) {
_transfer(token[i], from, to[i], amount[i]);
Expand All @@ -128,11 +113,9 @@ contract AllowanceTransfer is DomainSeparator {
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();
Expand All @@ -142,14 +125,11 @@ contract AllowanceTransfer is DomainSeparator {
}
}
}

// Transfer the tokens from the from address to the recipient.
ERC20(token).safeTransferFrom(from, to, amount);
}

/*//////////////////////////////////////////////////////////////
LOCKDOWN LOGIC
//////////////////////////////////////////////////////////////*/

// TODO: Bench if a struct for token-spender pairs is cheaper.
// TODO test
/// @notice Enables performing a "lockdown" of the sender's Permit2 identity
Expand All @@ -160,9 +140,7 @@ contract AllowanceTransfer is DomainSeparator {
/// Each index should correspond to an index in the tokens array.
function lockdown(address[] calldata tokens, address[] calldata spenders) external {
// Each index should correspond to an index in the other array.
if (tokens.length != spenders.length) {
revert LengthMismatch();
}
if (tokens.length != spenders.length) revert LengthMismatch();

// Revoke allowances for each pair of spenders and tokens.
unchecked {
Expand Down
22 changes: 9 additions & 13 deletions src/DomainSeparator.sol
Expand Up @@ -5,34 +5,30 @@ pragma solidity 0.8.17;
/// @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 DomainSeparator {
/* 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 =
keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)");

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);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed the this address(this) stuff isnt rly needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and removing it should give us a lil gas savings

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, @marktoda and I discussed this a bit more on Friday and think we should actually keep it. OZ decided to add this check (see convo here )so that delegatecalling would be supported safely and its minor gas overhead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why should we care about supporting delegatecalls at our own expense? this case is unique from OZ's convo because it is logical that tokens could want to delegatecall ERC20 impls with EIP712 mixed in, but no one should be delegatecaling permit2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's fair - happy to leave it out then

}

/// @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)));
}
Expand Down
12 changes: 6 additions & 6 deletions src/SignatureTransfer.sol
Expand Up @@ -143,11 +143,10 @@ contract SignatureTransfer is DomainSeparator {
) external {
_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();
}
if (requestedAmounts[i] > permit.signedAmounts[i]) revert InvalidAmount();
}
}

Expand Down Expand Up @@ -191,16 +190,17 @@ contract SignatureTransfer is DomainSeparator {
/// @notice Invalidates the bits specified in `mask` for the bitmap at `wordPos`.
function invalidateUnorderedNonces(uint256 wordPos, uint256 mask) external {
nonceBitmap[msg.sender][wordPos] |= mask;

emit InvalidateUnorderedNonces(msg.sender, wordPos, mask);
}

/// @notice Checks whether a nonce is taken. Then sets the bit at the bitPos in the bitmap at the wordPos.
function _useUnorderedNonce(address from, uint256 nonce) private {
(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);
}

Expand Down