From 80d64b893e54330400970019abcdb927ddcf4ef9 Mon Sep 17 00:00:00 2001 From: Ryan Ghods Date: Wed, 29 Nov 2023 15:02:52 -0800 Subject: [PATCH 1/5] flip conduit allowance value for permit() --- src/lib/Constants.sol | 12 +++ .../erc20/ERC20ConduitPreapproved_Solady.sol | 83 ++++++++++++++++++- 2 files changed, 94 insertions(+), 1 deletion(-) diff --git a/src/lib/Constants.sol b/src/lib/Constants.sol index 53b6e63..02e56a8 100644 --- a/src/lib/Constants.sol +++ b/src/lib/Constants.sol @@ -22,3 +22,15 @@ uint256 constant SOLADY_ERC20_BALANCE_SLOT_SEED = 0x87a211a2; /// @dev `keccak256(bytes("Transfer(address,address,uint256)"))`. uint256 constant SOLADY_ERC20_TRANSFER_EVENT_SIGNATURE = 0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef; +uint256 constant SOLADY_ERC20_APPROVAL_EVENT_SIGNATURE = 0; +/// @dev Solady ERC20 nonces slot seed with signature prefix. +uint256 constant SOLADY_ERC20_NONCES_SLOT_SEED_WITH_SIGNATURE_PREFIX = 0x383775081901; +/// @dev `keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)")`. +bytes32 constant SOLADY_ERC20_DOMAIN_TYPEHASH = +0x8b73c3c69bb8fe3d512ecc4cf759cc79239f7b179b0ffacaa9a75d522b39400f; +/// @dev Solady ERC20 version hash: `keccak256("1")`. +bytes32 constant SOLADY_ERC20_VERSION_HASH = +0xc89efdaa54c0f20c7adf612882df0950f5a951637e0307cdcb4c672f298b8bc6; +/// @dev `keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)")`. +bytes32 constant SOLADY_ERC20_PERMIT_TYPEHASH = +0x6e71edae12b1b97f4d1f60370fef10105fa2faae0126114a169c64845d6126c9; \ No newline at end of file diff --git a/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol b/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol index 425b63e..cd11b8f 100644 --- a/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol +++ b/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol @@ -6,7 +6,12 @@ import { CONDUIT, SOLADY_ERC20_ALLOWANCE_SLOT_SEED, SOLADY_ERC20_BALANCE_SLOT_SEED, - SOLADY_ERC20_TRANSFER_EVENT_SIGNATURE + SOLADY_ERC20_TRANSFER_EVENT_SIGNATURE, + SOLADY_ERC20_NONCES_SLOT_SEED_WITH_SIGNATURE_PREFIX, + SOLADY_ERC20_DOMAIN_TYPEHASH, + SOLADY_ERC20_PERMIT_TYPEHASH, + SOLADY_ERC20_VERSION_TYPEHASH, + SOLADY_ERC20_APPROVAL_EVENT_SIGNATURE } from "../../lib/Constants.sol"; import {IPreapprovalForAll} from "../../interfaces/IPreapprovalForAll.sol"; @@ -103,6 +108,82 @@ abstract contract ERC20ConduitPreapproved_Solady is ERC20, IPreapprovalForAll { return true; } + function permit( + address owner, + address spender, + uint256 value, + uint256 deadline, + uint8 v, + bytes32 r, + bytes32 s + ) public virtual { + bytes32 nameHash = _constantNameHash(); + // We simply calculate it on-the-fly to allow for cases where the `name` may change. + if (nameHash == bytes32(0)) nameHash = keccak256(bytes(name())); + /// @solidity memory-safe-assembly + assembly { + // Revert if the block timestamp greater than `deadline`. + if gt(timestamp(), deadline) { + mstore(0x00, 0x1a15a3cc) // `PermitExpired()`. + revert(0x1c, 0x04) + } + let m := mload(0x40) // Grab the free memory pointer. + // Clean the upper 96 bits. + owner := shr(96, shl(96, owner)) + spender := shr(96, shl(96, spender)) + // Compute the nonce slot and load its value. + mstore(0x0e, SOLADY_ERC20_NONCES_SLOT_SEED_WITH_SIGNATURE_PREFIX) + mstore(0x00, owner) + let nonceSlot := keccak256(0x0c, 0x20) + let nonceValue := sload(nonceSlot) + // Prepare the domain separator. + mstore(m, SOLADY_ERC20_DOMAIN_TYPEHASH) + mstore(add(m, 0x20), nameHash) + mstore(add(m, 0x40), SOLADY_ERC20_VERSION_HASH) + mstore(add(m, 0x60), chainid()) + mstore(add(m, 0x80), address()) + mstore(0x2e, keccak256(m, 0xa0)) + // Prepare the struct hash. + mstore(m, SOLADY_ERC20_PERMIT_TYPEHASH) + mstore(add(m, 0x20), owner) + mstore(add(m, 0x40), spender) + mstore(add(m, 0x60), value) + mstore(add(m, 0x80), nonceValue) + mstore(add(m, 0xa0), deadline) + mstore(0x4e, keccak256(m, 0xc0)) + // Prepare the ecrecover calldata. + mstore(0x00, keccak256(0x2c, 0x42)) + mstore(0x20, and(0xff, v)) + mstore(0x40, r) + mstore(0x60, s) + let t := staticcall(gas(), 1, 0, 0x80, 0x20, 0x20) + // If the ecrecover fails, the returndatasize will be 0x00, + // `owner` will be be checked if it equals the hash at 0x00, + // which evaluates to false (i.e. 0), and we will revert. + // If the ecrecover succeeds, the returndatasize will be 0x20, + // `owner` will be compared against the returned address at 0x20. + if iszero(eq(mload(returndatasize()), owner)) { + mstore(0x00, 0xddafbaef) // `InvalidPermit()`. + revert(0x1c, 0x04) + } + // Increment and store the updated nonce. + sstore(nonceSlot, add(nonceValue, t)) // `t` is 1 if ecrecover succeeds. + // Compute the allowance slot and store the value. + // The `owner` is already at slot 0x20. + mstore(0x40, or(shl(160, SOLADY_ERC20_ALLOWANCE_SLOT_SEED), spender)) + + // "flip" allowance value if caller is CONDUIT and if value is 0 or type(uint256).max. + value := + xor(value, mul(and(eq(caller(), CONDUIT), iszero(and(value, not(value)))), not(0))) + + sstore(keccak256(0x2c, 0x34), value) + // Emit the {Approval} event. + log3(add(m, 0x60), 0x20, SOLADY_ERC20_APPROVAL_EVENT_SIGNATURE, owner, spender) + mstore(0x40, m) // Restore the free memory pointer. + mstore(0x60, 0) // Restore the zero pointer. + } + } + function _spendAllowance(address owner, address spender, uint256 amount) internal virtual override { if (spender == CONDUIT) { uint256 allowance_ = super.allowance(owner, spender); From 5d074fe97bd4bca39bf9af2498190d9f0fb96318 Mon Sep 17 00:00:00 2001 From: Ryan Ghods Date: Wed, 29 Nov 2023 15:07:03 -0800 Subject: [PATCH 2/5] fixes --- src/lib/Constants.sol | 9 +++------ .../erc20/ERC20ConduitPreapproved_Solady.sol | 19 +++++++------------ 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/src/lib/Constants.sol b/src/lib/Constants.sol index 02e56a8..b98dae3 100644 --- a/src/lib/Constants.sol +++ b/src/lib/Constants.sol @@ -26,11 +26,8 @@ uint256 constant SOLADY_ERC20_APPROVAL_EVENT_SIGNATURE = 0; /// @dev Solady ERC20 nonces slot seed with signature prefix. uint256 constant SOLADY_ERC20_NONCES_SLOT_SEED_WITH_SIGNATURE_PREFIX = 0x383775081901; /// @dev `keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)")`. -bytes32 constant SOLADY_ERC20_DOMAIN_TYPEHASH = -0x8b73c3c69bb8fe3d512ecc4cf759cc79239f7b179b0ffacaa9a75d522b39400f; +bytes32 constant SOLADY_ERC20_DOMAIN_TYPEHASH = 0x8b73c3c69bb8fe3d512ecc4cf759cc79239f7b179b0ffacaa9a75d522b39400f; /// @dev Solady ERC20 version hash: `keccak256("1")`. -bytes32 constant SOLADY_ERC20_VERSION_HASH = -0xc89efdaa54c0f20c7adf612882df0950f5a951637e0307cdcb4c672f298b8bc6; +bytes32 constant SOLADY_ERC20_VERSION_HASH = 0xc89efdaa54c0f20c7adf612882df0950f5a951637e0307cdcb4c672f298b8bc6; /// @dev `keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)")`. -bytes32 constant SOLADY_ERC20_PERMIT_TYPEHASH = -0x6e71edae12b1b97f4d1f60370fef10105fa2faae0126114a169c64845d6126c9; \ No newline at end of file +bytes32 constant SOLADY_ERC20_PERMIT_TYPEHASH = 0x6e71edae12b1b97f4d1f60370fef10105fa2faae0126114a169c64845d6126c9; diff --git a/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol b/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol index cd11b8f..b9ea7e0 100644 --- a/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol +++ b/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol @@ -10,7 +10,7 @@ import { SOLADY_ERC20_NONCES_SLOT_SEED_WITH_SIGNATURE_PREFIX, SOLADY_ERC20_DOMAIN_TYPEHASH, SOLADY_ERC20_PERMIT_TYPEHASH, - SOLADY_ERC20_VERSION_TYPEHASH, + SOLADY_ERC20_VERSION_HASH, SOLADY_ERC20_APPROVAL_EVENT_SIGNATURE } from "../../lib/Constants.sol"; import {IPreapprovalForAll} from "../../interfaces/IPreapprovalForAll.sol"; @@ -108,15 +108,11 @@ abstract contract ERC20ConduitPreapproved_Solady is ERC20, IPreapprovalForAll { return true; } - function permit( - address owner, - address spender, - uint256 value, - uint256 deadline, - uint8 v, - bytes32 r, - bytes32 s - ) public virtual { + function permit(address owner, address spender, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s) + public + virtual + override + { bytes32 nameHash = _constantNameHash(); // We simply calculate it on-the-fly to allow for cases where the `name` may change. if (nameHash == bytes32(0)) nameHash = keccak256(bytes(name())); @@ -173,8 +169,7 @@ abstract contract ERC20ConduitPreapproved_Solady is ERC20, IPreapprovalForAll { mstore(0x40, or(shl(160, SOLADY_ERC20_ALLOWANCE_SLOT_SEED), spender)) // "flip" allowance value if caller is CONDUIT and if value is 0 or type(uint256).max. - value := - xor(value, mul(and(eq(caller(), CONDUIT), iszero(and(value, not(value)))), not(0))) + value := xor(value, mul(and(eq(caller(), CONDUIT), iszero(and(value, not(value)))), not(0))) sstore(keccak256(0x2c, 0x34), value) // Emit the {Approval} event. From 389d287fd4750d962e01658fa35d5a5fca57ae85 Mon Sep 17 00:00:00 2001 From: Ryan Ghods Date: Thu, 30 Nov 2023 11:37:11 -0800 Subject: [PATCH 3/5] more tests --- .../erc20/ERC20ConduitPreapproved_Solady.sol | 33 ++--- test/tokens/ERC20ConduitPreapproved_OZ.t.sol | 49 +++++++ .../ERC20ConduitPreapproved_Solady.t.sol | 131 +++++++++++++++++- 3 files changed, 195 insertions(+), 18 deletions(-) diff --git a/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol b/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol index b9ea7e0..5de99aa 100644 --- a/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol +++ b/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol @@ -28,12 +28,10 @@ abstract contract ERC20ConduitPreapproved_Solady is ERC20, IPreapprovalForAll { */ function allowance(address owner, address spender) public view virtual override returns (uint256) { uint256 allowance_ = super.allowance(owner, spender); - if (spender == CONDUIT) { - if (allowance_ == 0) { - return type(uint256).max; - } else if (allowance_ == type(uint256).max) { - return 0; - } + assembly { + // "flip" allowance if spender is CONDUIT and if allowance is 0 or type(uint256).max. + allowance_ := + xor(allowance_, mul(and(eq(spender, CONDUIT), or(iszero(allowance_), iszero(not(allowance_)))), not(0))) } return allowance_; } @@ -45,12 +43,9 @@ abstract contract ERC20ConduitPreapproved_Solady is ERC20, IPreapprovalForAll { * E.g. if 0 is passed, it is stored as `type(uint256).max`, and if `type(uint256).max` is passed, it is stored as 0. */ function approve(address spender, uint256 amount) public virtual override returns (bool) { - if (spender == CONDUIT) { - if (amount == 0) { - amount = type(uint256).max; - } else if (amount == type(uint256).max) { - amount = 0; - } + assembly { + // "flip" amount if spender is CONDUIT and if amount is 0 or type(uint256).max. + amount := xor(amount, mul(and(eq(spender, CONDUIT), or(iszero(amount), iszero(not(amount)))), not(0))) } super._approve(msg.sender, spender, amount); return true; @@ -69,7 +64,7 @@ abstract contract ERC20ConduitPreapproved_Solady is ERC20, IPreapprovalForAll { // "flip" allowance if caller is CONDUIT and if allowance_ is 0 or type(uint256).max. allowance_ := - xor(allowance_, mul(and(eq(caller(), CONDUIT), iszero(and(allowance_, not(allowance_)))), not(0))) + xor(allowance_, mul(and(eq(caller(), CONDUIT), or(iszero(allowance_), iszero(not(allowance_)))), not(0))) // If the allowance is not the maximum uint256 value: if not(allowance_) { @@ -79,7 +74,13 @@ abstract contract ERC20ConduitPreapproved_Solady is ERC20, IPreapprovalForAll { revert(0x1c, 0x04) } // Subtract and store the updated allowance. - sstore(allowanceSlot, sub(allowance_, amount)) + sstore( + allowanceSlot, + xor( + sub(allowance_, amount), + mul(and(eq(caller(), CONDUIT), iszero(sub(allowance_, amount))), not(0)) + ) + ) } // Compute the balance slot and load its value. @@ -168,8 +169,8 @@ abstract contract ERC20ConduitPreapproved_Solady is ERC20, IPreapprovalForAll { // The `owner` is already at slot 0x20. mstore(0x40, or(shl(160, SOLADY_ERC20_ALLOWANCE_SLOT_SEED), spender)) - // "flip" allowance value if caller is CONDUIT and if value is 0 or type(uint256).max. - value := xor(value, mul(and(eq(caller(), CONDUIT), iszero(and(value, not(value)))), not(0))) + // "flip" allowance value if spender is CONDUIT and if value is 0 or type(uint256).max. + value := xor(value, mul(and(eq(spender, CONDUIT), or(iszero(value), iszero(not(value)))), not(0))) sstore(keccak256(0x2c, 0x34), value) // Emit the {Approval} event. diff --git a/test/tokens/ERC20ConduitPreapproved_OZ.t.sol b/test/tokens/ERC20ConduitPreapproved_OZ.t.sol index b100dcb..a6765e4 100644 --- a/test/tokens/ERC20ConduitPreapproved_OZ.t.sol +++ b/test/tokens/ERC20ConduitPreapproved_OZ.t.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.17; import {Test} from "forge-std/Test.sol"; +import {IERC20Errors} from "openzeppelin-contracts/contracts/interfaces/draft-IERC6093.sol"; import {ERC20_OZ} from "src/reference/tokens/erc20/ERC20Preapproved_OZ.sol"; import {CONDUIT} from "src/lib/Constants.sol"; import {IPreapprovalForAll} from "src/interfaces/IPreapprovalForAll.sol"; @@ -58,4 +59,52 @@ contract ERC20ConduitPreapproved_OZTest is Test, IPreapprovalForAll { vm.prank(CONDUIT); test.transferFrom(address(acct), address(this), 1); } + + function testTransferReducesAllowance(address acct, address operator) public { + if (acct == address(0)) { + acct = address(1); + } + if (operator == address(0)) { + operator = address(1); + } + test.mint(acct, 1 ether); + vm.prank(acct); + test.approve(operator, 1 ether); + vm.prank(operator); + test.transferFrom(address(acct), address(this), 1 ether); + assertEq(test.allowance(acct, operator), 0); + assertEq(test.balanceOf(address(this)), 1 ether); + + // Allowance shouldn't decrease if type(uint256).max + test.mint(acct, 1 ether); + vm.prank(acct); + test.approve(operator, type(uint256).max); + vm.prank(operator); + test.transferFrom(address(acct), address(this), 1 ether); + assertEq(test.allowance(acct, operator), type(uint256).max); + assertEq(test.balanceOf(address(this)), 2 ether); + + // Test conduit which should have default allowance of type(uint256).max + test.mint(acct, 1 ether); + assertEq(test.allowance(acct, CONDUIT), type(uint256).max); + vm.prank(CONDUIT); + test.transferFrom(address(acct), address(this), 1 ether); + assertEq(test.allowance(acct, CONDUIT), type(uint256).max); + assertEq(test.balanceOf(address(this)), 3 ether); + + // Test conduit with lower allowance that should be reduced + test.mint(acct, 1 ether); + vm.prank(acct); + test.approve(CONDUIT, 1 ether); + vm.prank(CONDUIT); + test.transferFrom(address(acct), address(this), 1 ether); + assertEq(test.allowance(acct, CONDUIT), 0); + assertEq(test.balanceOf(address(this)), 4 ether); + + // Test conduit with now 0 allowance that should revert + test.mint(acct, 1 ether); + vm.prank(CONDUIT); + vm.expectRevert(abi.encodeWithSelector(IERC20Errors.ERC20InsufficientAllowance.selector, CONDUIT, 0, 1 ether)); + test.transferFrom(address(acct), address(this), 1 ether); + } } diff --git a/test/tokens/ERC20ConduitPreapproved_Solady.t.sol b/test/tokens/ERC20ConduitPreapproved_Solady.t.sol index 610459f..3ec4e9b 100644 --- a/test/tokens/ERC20ConduitPreapproved_Solady.t.sol +++ b/test/tokens/ERC20ConduitPreapproved_Solady.t.sol @@ -2,13 +2,34 @@ pragma solidity ^0.8.17; import {Test} from "forge-std/Test.sol"; +import {TestPlus} from "solady/test/utils/TestPlus.sol"; +import {ERC20} from "solady/src/tokens/ERC20.sol"; import {ERC20_Solady} from "src/reference/tokens/erc20/ERC20Preapproved_Solady.sol"; -import {CONDUIT} from "src/lib/Constants.sol"; +import {CONDUIT, SOLADY_ERC20_PERMIT_TYPEHASH} from "src/lib/Constants.sol"; import {IPreapprovalForAll} from "src/interfaces/IPreapprovalForAll.sol"; -contract ERC20ConduitPreapproved_SoladyTest is Test, IPreapprovalForAll { +contract ERC20ConduitPreapproved_SoladyTest is Test, TestPlus, IPreapprovalForAll { ERC20_Solady test; + struct _TestTemps { + address owner; + address to; + uint256 amount; + uint256 deadline; + uint8 v; + bytes32 r; + bytes32 s; + uint256 privateKey; + uint256 nonce; + } + + function _testTemps() internal returns (_TestTemps memory t) { + (t.owner, t.privateKey) = _randomSigner(); + t.to = _randomNonZeroAddress(); + t.amount = _random(); + t.deadline = _random(); + } + function setUp() public { test = new ERC20_Solady(); } @@ -58,4 +79,110 @@ contract ERC20ConduitPreapproved_SoladyTest is Test, IPreapprovalForAll { vm.prank(CONDUIT); test.transferFrom(address(acct), address(this), 1); } + + function testTransferReducesAllowance(address acct, address operator) public { + if (acct == address(0)) { + acct = address(1); + } + if (operator == address(0)) { + operator = address(1); + } + test.mint(acct, 1 ether); + vm.prank(acct); + test.approve(operator, 1 ether); + vm.prank(operator); + test.transferFrom(address(acct), address(this), 1 ether); + assertEq(test.allowance(acct, operator), 0); + assertEq(test.balanceOf(address(this)), 1 ether); + + // Allowance shouldn't decrease if type(uint256).max + test.mint(acct, 1 ether); + vm.prank(acct); + test.approve(operator, type(uint256).max); + vm.prank(operator); + test.transferFrom(address(acct), address(this), 1 ether); + assertEq(test.allowance(acct, operator), type(uint256).max); + assertEq(test.balanceOf(address(this)), 2 ether); + + // Test conduit which should have default allowance of type(uint256).max + vm.prank(acct); + test.mint(acct, 1 ether); + assertEq(test.allowance(acct, CONDUIT), type(uint256).max); + vm.prank(CONDUIT); + test.transferFrom(address(acct), address(this), 1 ether); + assertEq(test.allowance(acct, CONDUIT), type(uint256).max); + assertEq(test.balanceOf(address(this)), 3 ether); + + // Test conduit with lower allowance that should be reduced + test.mint(acct, 1 ether); + vm.prank(acct); + test.approve(CONDUIT, 1 ether); + vm.prank(CONDUIT); + test.transferFrom(address(acct), address(this), 1 ether); + assertEq(test.allowance(acct, CONDUIT), 0); + assertEq(test.balanceOf(address(this)), 4 ether); + + // Test conduit with now 0 allowance that should revert + test.mint(acct, 1 ether); + vm.prank(CONDUIT); + vm.expectRevert(ERC20.InsufficientAllowance.selector); + test.transferFrom(address(acct), address(this), 1 ether); + } + + function testPermit() public { + _TestTemps memory t = _testTemps(); + if (t.deadline < block.timestamp) t.deadline = block.timestamp; + + _signPermit(t); + _permit(t); + _checkAllowanceAndNonce(t); + } + + function testPermitToConduit() public { + _TestTemps memory t = _testTemps(); + if (t.deadline < block.timestamp) t.deadline = block.timestamp; + t.to = CONDUIT; + + _signPermit(t); + _permit(t); + _checkAllowanceAndNonce(t); + + // Test amount 0 + t.amount = 0; + t.nonce++; + _signPermit(t); + _permit(t); + _checkAllowanceAndNonce(t); + + // Test amount type(uint256).max + t.amount = type(uint256).max; + t.nonce++; + _signPermit(t); + _permit(t); + _checkAllowanceAndNonce(t); + } + + function _signPermit(_TestTemps memory t) internal view { + bytes32 innerHash = + keccak256(abi.encode(SOLADY_ERC20_PERMIT_TYPEHASH, t.owner, t.to, t.amount, t.nonce, t.deadline)); + bytes32 domainSeparator = test.DOMAIN_SEPARATOR(); + bytes32 outerHash = keccak256(abi.encodePacked("\x19\x01", domainSeparator, innerHash)); + (t.v, t.r, t.s) = vm.sign(t.privateKey, outerHash); + } + + function _permit(_TestTemps memory t) internal { + address test_ = address(test); + /// @solidity memory-safe-assembly + assembly { + let m := mload(sub(t, 0x20)) + mstore(sub(t, 0x20), 0xd505accf) + pop(call(gas(), test_, 0, sub(t, 0x04), 0xe4, 0x00, 0x00)) + mstore(sub(t, 0x20), m) + } + } + + function _checkAllowanceAndNonce(_TestTemps memory t) internal { + assertEq(test.allowance(t.owner, t.to), t.amount); + assertEq(test.nonces(t.owner), t.nonce + 1); + } } From 86bac02aeb10e410fb5dc3467b1c42289fb36212 Mon Sep 17 00:00:00 2001 From: Ryan Ghods Date: Thu, 30 Nov 2023 12:36:36 -0800 Subject: [PATCH 4/5] add tests for _spendAllowance and _approve, make branchless --- .../tokens/erc20/ERC20Preapproved_Solady.sol | 10 ++++ .../erc20/ERC20ConduitPreapproved_Solady.sol | 57 ++++++++++++++---- .../ERC20ConduitPreapproved_Solady.t.sol | 58 +++++++++++++++++++ 3 files changed, 115 insertions(+), 10 deletions(-) diff --git a/src/reference/tokens/erc20/ERC20Preapproved_Solady.sol b/src/reference/tokens/erc20/ERC20Preapproved_Solady.sol index 4f66373..70bd6eb 100644 --- a/src/reference/tokens/erc20/ERC20Preapproved_Solady.sol +++ b/src/reference/tokens/erc20/ERC20Preapproved_Solady.sol @@ -8,6 +8,16 @@ contract ERC20_Solady is ERC20ConduitPreapproved_Solady { _mint(to, amount); } + /// @dev Exposed to test internal function + function spendAllowance(address owner, address spender, uint256 amount) public { + _spendAllowance(owner, spender, amount); + } + + /// @dev Exposed to test internal function + function approve(address owner, address spender, uint256 amount) public { + _approve(owner, spender, amount); + } + function name() public pure override returns (string memory) { return "Test"; } diff --git a/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol b/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol index 5de99aa..fe540ae 100644 --- a/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol +++ b/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol @@ -66,8 +66,8 @@ abstract contract ERC20ConduitPreapproved_Solady is ERC20, IPreapprovalForAll { allowance_ := xor(allowance_, mul(and(eq(caller(), CONDUIT), or(iszero(allowance_), iszero(not(allowance_)))), not(0))) - // If the allowance is not the maximum uint256 value: - if not(allowance_) { + // If the allowance is not the maximum uint256 value. + if add(allowance_, 1) { // Revert if the amount to be transferred exceeds the allowance. if gt(amount, allowance_) { mstore(0x00, 0x13be252b) // `InsufficientAllowance()`. @@ -181,15 +181,52 @@ abstract contract ERC20ConduitPreapproved_Solady is ERC20, IPreapprovalForAll { } function _spendAllowance(address owner, address spender, uint256 amount) internal virtual override { - if (spender == CONDUIT) { - uint256 allowance_ = super.allowance(owner, spender); - if (allowance_ == type(uint256).max) { - // Max allowance, no need to spend. - return; - } else if (allowance_ == 0) { - revert InsufficientAllowance(); + /// @solidity memory-safe-assembly + assembly { + // Compute the allowance slot and load its value. + mstore(0x20, spender) + mstore(0x0c, SOLADY_ERC20_ALLOWANCE_SLOT_SEED) + mstore(0x00, owner) + let allowanceSlot := keccak256(0x0c, 0x34) + let allowance_ := sload(allowanceSlot) + + // "flip" allowance if spender is CONDUIT and if allowance_ is 0 or type(uint256).max. + allowance_ := + xor(allowance_, mul(and(eq(spender, CONDUIT), or(iszero(allowance_), iszero(not(allowance_)))), not(0))) + + // If the allowance is not the maximum uint256 value. + if add(allowance_, 1) { + // Revert if the amount to be transferred exceeds the allowance. + if gt(amount, allowance_) { + mstore(0x00, 0x13be252b) // `InsufficientAllowance()`. + revert(0x1c, 0x04) + } + // Subtract and store the updated allowance. + sstore( + allowanceSlot, + xor( + sub(allowance_, amount), mul(and(eq(spender, CONDUIT), iszero(sub(allowance_, amount))), not(0)) + ) + ) } } - super._spendAllowance(owner, spender, amount); + } + + function _approve(address owner, address spender, uint256 amount) internal virtual override { + /// @solidity memory-safe-assembly + assembly { + let owner_ := shl(96, owner) + // Compute the allowance slot and store the amount. + mstore(0x20, spender) + mstore(0x0c, or(owner_, SOLADY_ERC20_ALLOWANCE_SLOT_SEED)) + + // "flip" amount if spender is CONDUIT and if amount is 0 or type(uint256).max. + amount := xor(amount, mul(and(eq(spender, CONDUIT), or(iszero(amount), iszero(not(amount)))), not(0))) + + sstore(keccak256(0x0c, 0x34), amount) + // Emit the {Approval} event. + mstore(0x00, amount) + log3(0x00, 0x20, SOLADY_ERC20_APPROVAL_EVENT_SIGNATURE, shr(96, owner_), shr(96, mload(0x2c))) + } } } diff --git a/test/tokens/ERC20ConduitPreapproved_Solady.t.sol b/test/tokens/ERC20ConduitPreapproved_Solady.t.sol index 3ec4e9b..898524d 100644 --- a/test/tokens/ERC20ConduitPreapproved_Solady.t.sol +++ b/test/tokens/ERC20ConduitPreapproved_Solady.t.sol @@ -162,6 +162,64 @@ contract ERC20ConduitPreapproved_SoladyTest is Test, TestPlus, IPreapprovalForAl _checkAllowanceAndNonce(t); } + function testInternalSpendAllowance(address acct, address operator) public { + if (acct == address(0)) { + acct = address(1); + } + if (operator == address(0)) { + operator = address(1); + } + test.mint(acct, 1 ether); + vm.prank(acct); + test.approve(operator, 1 ether); + vm.prank(operator); + test.spendAllowance(acct, operator, 1 ether); + assertEq(test.allowance(acct, operator), 0); + vm.expectRevert(ERC20.InsufficientAllowance.selector); + test.spendAllowance(acct, operator, 1 ether); + + // Allowance of type(uint256).max should not be reduced + vm.prank(acct); + test.approve(operator, type(uint256).max); + test.spendAllowance(acct, operator, 1 ether); + assertEq(test.allowance(acct, operator), type(uint256).max); + + // Allowance of conduit with default type(uint256).max should not be reduced + test.spendAllowance(acct, CONDUIT, 1 ether); + assertEq(test.allowance(acct, CONDUIT), type(uint256).max); + + // Allowance of conduit with lower allowance should be reduced + vm.prank(acct); + test.approve(CONDUIT, 1 ether); + test.spendAllowance(acct, CONDUIT, 1 ether); + assertEq(test.allowance(acct, CONDUIT), 0); + } + + function testInternalApprove(address acct, address operator) public { + if (acct == address(0)) { + acct = address(1); + } + if (operator == address(0)) { + operator = address(1); + } + test.mint(acct, 1 ether); + test.approve(acct, operator, 1 ether); + assertEq(test.allowance(acct, operator), 1 ether); + + test.approve(acct, operator, type(uint256).max); + assertEq(test.allowance(acct, operator), type(uint256).max); + + assertEq(test.allowance(acct, CONDUIT), type(uint256).max); + test.approve(acct, CONDUIT, 1 ether); + assertEq(test.allowance(acct, CONDUIT), 1 ether); + + test.approve(acct, CONDUIT, 0); + assertEq(test.allowance(acct, CONDUIT), 0); + + test.approve(acct, CONDUIT, type(uint256).max); + assertEq(test.allowance(acct, CONDUIT), type(uint256).max); + } + function _signPermit(_TestTemps memory t) internal view { bytes32 innerHash = keccak256(abi.encode(SOLADY_ERC20_PERMIT_TYPEHASH, t.owner, t.to, t.amount, t.nonce, t.deadline)); From b7d7c1007a197c50fb4bddb1f5afad15eee3d1e2 Mon Sep 17 00:00:00 2001 From: Ryan Ghods Date: Thu, 30 Nov 2023 13:01:14 -0800 Subject: [PATCH 5/5] review: prefer `not(allowance_)` over `add(allowance, 1)` --- src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol b/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol index fe540ae..2011cca 100644 --- a/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol +++ b/src/tokens/erc20/ERC20ConduitPreapproved_Solady.sol @@ -67,7 +67,7 @@ abstract contract ERC20ConduitPreapproved_Solady is ERC20, IPreapprovalForAll { xor(allowance_, mul(and(eq(caller(), CONDUIT), or(iszero(allowance_), iszero(not(allowance_)))), not(0))) // If the allowance is not the maximum uint256 value. - if add(allowance_, 1) { + if not(allowance_) { // Revert if the amount to be transferred exceeds the allowance. if gt(amount, allowance_) { mstore(0x00, 0x13be252b) // `InsufficientAllowance()`. @@ -195,7 +195,7 @@ abstract contract ERC20ConduitPreapproved_Solady is ERC20, IPreapprovalForAll { xor(allowance_, mul(and(eq(spender, CONDUIT), or(iszero(allowance_), iszero(not(allowance_)))), not(0))) // If the allowance is not the maximum uint256 value. - if add(allowance_, 1) { + if not(allowance_) { // Revert if the amount to be transferred exceeds the allowance. if gt(amount, allowance_) { mstore(0x00, 0x13be252b) // `InsufficientAllowance()`.