From 69dda7e1c1baa8fd5a4be27fef6282228ced2c99 Mon Sep 17 00:00:00 2001
From: Wilson Cusack <wilson.cusack@coinbase.com>
Date: Wed, 3 Apr 2024 16:44:01 -0400
Subject: [PATCH 1/5] Guard against concurrent withdraws causing unexpected
 reverts

---
 .gas-snapshot                      | 54 ++++++++++++++++------------
 script/DeployMagicSpend.s.sol      |  2 +-
 src/MagicSpend.sol                 | 57 ++++++++++++++++++++----------
 test/MagicSpend.t.sol              |  2 +-
 test/PostOp.t.sol                  |  2 +-
 test/Validate.t.sol                | 28 +++++++++++++++
 test/ValidatePaymasterUserOp.t.sol | 16 +++++----
 test/Withdraw.t.sol                |  1 +
 test/echidna/FuzzSetup.sol         |  2 +-
 9 files changed, 113 insertions(+), 51 deletions(-)

diff --git a/.gas-snapshot b/.gas-snapshot
index 5081718..d7cd5a7 100644
--- a/.gas-snapshot
+++ b/.gas-snapshot
@@ -1,27 +1,35 @@
-PostOpTest:test_RevertsIfPostOpFailed(uint256,uint256,uint256) (runs: 256, μ: 94981, ~: 97095)
+GetHashTest:test_returnsValidHash() (gas: 46531)
+IsValidWithdrawalSignature:test_returnsFalseWithInvalidSignature() (gas: 53845)
+IsValidWithdrawalSignature:test_returnsTrueWithValidSignature() (gas: 51016)
+OwnerWithdrawTest:test_revertsIfNotOwner() (gas: 15305)
+OwnerWithdrawTest:test_transfersERC20Successfully(uint256) (runs: 256, μ: 97271, ~: 99711)
+OwnerWithdrawTest:test_transfersETHSuccessfully(uint256) (runs: 256, μ: 51418, ~: 53415)
+PostOpTest:test_RevertsIfPostOpFailed(uint256,uint256,uint256) (runs: 256, μ: 97167, ~: 99371)
 PostOpTest:test_entryPointAddStake(uint112,uint32) (runs: 256, μ: 56233, ~: 56233)
-PostOpTest:test_entryPointDeposit(uint112) (runs: 256, μ: 32331, ~: 32962)
-PostOpTest:test_entryPointUnlockStake() (gas: 54910)
-PostOpTest:test_entryPointWithdraw(uint112) (runs: 256, μ: 63500, ~: 65300)
-PostOpTest:test_entryPointWithdrawStake() (gas: 71825)
-PostOpTest:test_paymasterPaysForOp() (gas: 208402)
-PostOpTest:test_transfersExcess(uint256,uint256,uint256,uint256) (runs: 256, μ: 110216, ~: 111227)
+PostOpTest:test_entryPointDeposit(uint112) (runs: 256, μ: 32516, ~: 32962)
+PostOpTest:test_entryPointUnlockStake() (gas: 54888)
+PostOpTest:test_entryPointWithdraw(uint112) (runs: 256, μ: 63200, ~: 65300)
+PostOpTest:test_entryPointWithdrawStake() (gas: 71803)
+PostOpTest:test_paymasterPaysForOp() (gas: 210663)
+PostOpTest:test_transfersExcess(uint256,uint256,uint256,uint256) (runs: 256, μ: 112465, ~: 113488)
 Simulate:test() (gas: 12114)
-ValidatePaymasterUserOpTest:test_emitsCorrectly(address,uint256,uint256) (runs: 256, μ: 102564, ~: 103835)
-ValidatePaymasterUserOpTest:test_recordsNonceUsed(uint256) (runs: 256, μ: 109936, ~: 111025)
-ValidatePaymasterUserOpTest:test_returns1sIfWrongSignature() (gas: 94076)
-ValidatePaymasterUserOpTest:test_returnsCorrectly() (gas: 88920)
+ValidatePaymasterUserOpTest:test_emitsCorrectly(address,uint256,uint256) (runs: 256, μ: 104847, ~: 106080)
+ValidatePaymasterUserOpTest:test_recordsNonceUsed(uint256) (runs: 256, μ: 112158, ~: 113247)
+ValidatePaymasterUserOpTest:test_returns1sIfWrongSignature() (gas: 96298)
+ValidatePaymasterUserOpTest:test_returnsCorrectly() (gas: 91185)
 ValidatePaymasterUserOpTest:test_revertsIfMaxCostMoreThanRequested() (gas: 39141)
-ValidatePaymasterUserOpTest:test_revertsIfNonceUsed() (gas: 101280)
-ValidatePaymasterUserOpTest:test_revertsIfWithdrawAssetNotZero() (gas: 55974)
+ValidatePaymasterUserOpTest:test_revertsIfNonceUsed() (gas: 103502)
+ValidatePaymasterUserOpTest:test_revertsIfWithdrawAssetNotZero() (gas: 55952)
+ValidatePaymasterUserOpTest:test_reverts_whenWithdrawExceedsMaxAllowed(uint256,uint256,uint256) (runs: 256, μ: 100811, ~: 100992)
 ValidateTest:test_receive() (gas: 14687)
-WithdrawGasExcess:test_RevertsIfNoExcess(uint256) (runs: 256, μ: 68914, ~: 69177)
-WithdrawGasExcess:test_transferExcess(uint256,uint256,uint256) (runs: 256, μ: 102074, ~: 102074)
-WithdrawTest:test_emitsCorrectly(address,uint256,uint256) (runs: 256, μ: 115868, ~: 119073)
-WithdrawTest:test_emitsCorrectlyERC20Withdraw(address,uint256,uint256) (runs: 256, μ: 158052, ~: 160513)
-WithdrawTest:test_recordsNonceUsed(uint256) (runs: 256, μ: 109078, ~: 110167)
-WithdrawTest:test_revertsIfExpired(uint48,uint256) (runs: 256, μ: 59403, ~: 59535)
-WithdrawTest:test_revertsIfNonceUsed() (gas: 93506)
-WithdrawTest:test_revertsIfWrongSignature() (gas: 61536)
-WithdrawTest:test_transfersERC20Successfully(uint256) (runs: 256, μ: 136385, ~: 138825)
-WithdrawTest:test_transfersETHSuccessfully(uint256) (runs: 256, μ: 89398, ~: 91395)
\ No newline at end of file
+WithdrawGasExcess:test_RevertsIfNoExcess(uint256) (runs: 256, μ: 71133, ~: 71396)
+WithdrawGasExcess:test_transferExcess(uint256,uint256,uint256) (runs: 256, μ: 104279, ~: 104290)
+WithdrawTest:test_emitsCorrectly(address,uint256,uint256) (runs: 256, μ: 118413, ~: 121368)
+WithdrawTest:test_emitsCorrectlyERC20Withdraw(address,uint256,uint256) (runs: 256, μ: 162446, ~: 164773)
+WithdrawTest:test_recordsNonceUsed(uint256) (runs: 256, μ: 111372, ~: 112461)
+WithdrawTest:test_revertsIfExpired(uint48,uint256) (runs: 256, μ: 61664, ~: 61785)
+WithdrawTest:test_revertsIfNonceUsed() (gas: 95844)
+WithdrawTest:test_revertsIfWrongSignature() (gas: 63830)
+WithdrawTest:test_reverts_whenWithdrawExceedsMaxAllowed(uint256,uint256,uint256) (runs: 256, μ: 85124, ~: 85096)
+WithdrawTest:test_transfersERC20Successfully(uint256) (runs: 256, μ: 140626, ~: 143066)
+WithdrawTest:test_transfersETHSuccessfully(uint256) (runs: 256, μ: 91692, ~: 93689)
\ No newline at end of file
diff --git a/script/DeployMagicSpend.s.sol b/script/DeployMagicSpend.s.sol
index f0bc9d7..b00418c 100644
--- a/script/DeployMagicSpend.s.sol
+++ b/script/DeployMagicSpend.s.sol
@@ -12,7 +12,7 @@ contract MagicSpendDeployScript is Script {
         address signerAddress = 0x3E0cd4Dc43811888efa242Ab17118FcE0035EFF7;
         uint256 deployerPrivateKey = vm.envUint("PRIVATE_KEY");
         vm.startBroadcast(deployerPrivateKey);
-        MagicSpend c = new MagicSpend{salt: "0x1"}(vm.addr(deployerPrivateKey));
+        MagicSpend c = new MagicSpend{salt: "0x1"}(vm.addr(deployerPrivateKey), 20);
         console2.log(address(c));
         c.entryPointDeposit{value: 0.01 ether}(0.01 ether);
         c.entryPointAddStake{value: 0x16345785d8a0000}(0x16345785d8a0000, 0x15180);
diff --git a/src/MagicSpend.sol b/src/MagicSpend.sol
index c2023e6..aaebb53 100644
--- a/src/MagicSpend.sol
+++ b/src/MagicSpend.sol
@@ -30,6 +30,10 @@ contract MagicSpend is Ownable, IPaymaster {
         uint48 expiry;
     }
 
+    /// @notice The maximum percentage of address.balance a single withdraw.
+    /// @dev Helps prevent withdraws in the same transaction leading to reverts and hurting paymaster reputation.
+    uint256 public maxWithdrawDenominator;
+
     /// @notice Track the ETH available to be withdrawn per user.
     mapping(address user => uint256 amount) internal _withdrawableETH;
 
@@ -44,6 +48,11 @@ contract MagicSpend is Ownable, IPaymaster {
     /// @param nonce   The request nonce.
     event MagicSpendWithdrawal(address indexed account, address indexed asset, uint256 amount, uint256 nonce);
 
+    /// @notice Emitted when the `maxWithdrawDenominator` is set.
+    ///
+    /// @param newDenominator The new maxWithdrawDenominator value.
+    event MaxWithdrawDenominatorSet(uint256 newDenominator);
+
     /// @notice Thrown when the withdraw request signature is invalid.
     ///
     /// @dev The withdraw request signature MUST be:
@@ -72,12 +81,11 @@ contract MagicSpend is Ownable, IPaymaster {
     /// @param asset The requested asset.
     error UnsupportedPaymasterAsset(address asset);
 
-    /// @notice Thrown during `UserOperation` validation when the current balance is insufficient to cover the
-    ///         requested amount (exluding the `maxGasCost` set by the Entrypoint).
+    /// @notice Thrown if WithdrawRequest.amount exceeds address(this).balance / maxWithdrawDenominator.
     ///
     /// @param requestedAmount The requested amount excluding gas.
-    /// @param balance         The current contract balance.
-    error InsufficientBalance(uint256 requestedAmount, uint256 balance);
+    /// @param maxAllowed      The current max allowed withdraw.
+    error WithdrawTooLarge(uint256 requestedAmount, uint256 maxAllowed);
 
     /// @notice Thrown when trying to withdraw funds but nothing is available.
     error NoExcess();
@@ -97,9 +105,10 @@ contract MagicSpend is Ownable, IPaymaster {
 
     /// @notice Deploy the contract and set its initial owner.
     ///
-    /// @param _owner The initial owner of this contract.
-    constructor(address _owner) {
-        Ownable._initializeOwner(_owner);
+    /// @param owner_ The initial owner of this contract.
+    constructor(address owner_, uint256 maxWithdrawDenominator_) {
+        Ownable._initializeOwner(owner_);
+        _setMaxWithdrawDenominator(maxWithdrawDenominator_);
     }
 
     /// @notice Receive function allowing ETH to be deposited in this contract.
@@ -127,13 +136,6 @@ contract MagicSpend is Ownable, IPaymaster {
         bool sigFailed = !isValidWithdrawSignature(userOp.sender, withdrawRequest);
         validationData = (sigFailed ? 1 : 0) | (uint256(withdrawRequest.expiry) << 160);
 
-        // Ensure at validation that the contract has enough balance to cover the requested funds.
-        // NOTE: This check is necessary to enforce that the contract will be able to transfer the remaining funds
-        //       when `postOp()` is called back after the `UserOperation` has been executed.
-        if (address(this).balance < withdrawAmount) {
-            revert InsufficientBalance(withdrawAmount, address(this).balance);
-        }
-
         // NOTE: Do not include the gas part in withdrawable funds as it will be handled in `postOp()`.
         _withdrawableETH[userOp.sender] += withdrawAmount - maxCost;
         context = abi.encode(maxCost, userOp.sender);
@@ -144,10 +146,14 @@ contract MagicSpend is Ownable, IPaymaster {
         external
         onlyEntryPoint
     {
-        // `PostOpMode.postOpReverted` should be impossible.
-        // Only possible cause would be if this contract does not own enough ETH to transfer
-        // but this is checked at the validation step.
-        assert(mode != PostOpMode.postOpReverted);
+        // `PostOpMode.postOpReverted` should never happen.
+        // The flow here can only revert if there are > maxWithdrawDenominator
+        // withdraws in the same transaction, which should be highly unlikely.
+        // If the ETH transfer fails, the entire bundle will revert due an issue in the EntryPoint
+        // https://github.com/eth-infinitism/account-abstraction/pull/293
+        if (mode == PostOpMode.postOpReverted) {
+            revert UnexpectedPostOpRevertedMode();
+        }
 
         (uint256 maxGasCost, address account) = abi.decode(context, (uint256, address));
 
@@ -249,6 +255,10 @@ contract MagicSpend is Ownable, IPaymaster {
         IEntryPoint(entryPoint()).withdrawStake(to);
     }
 
+    function setMaxWithdrawDenominator(uint256 newDenominator) external onlyOwner {
+        _setMaxWithdrawDenominator(newDenominator);
+    }
+
     /// @notice Returns whether the `withdrawRequest` signature is valid for the given `account`.
     ///
     /// @dev Does not validate nonce or expiry.
@@ -305,6 +315,12 @@ contract MagicSpend is Ownable, IPaymaster {
         return 0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789;
     }
 
+    function _setMaxWithdrawDenominator(uint256 newDenominator) internal {
+        maxWithdrawDenominator = newDenominator;
+
+        emit MaxWithdrawDenominatorSet(newDenominator);
+    }
+
     /// @notice Validate the `withdrawRequest` against the given `account`.
     ///
     /// @dev Runs all non-signature validation checks.
@@ -317,6 +333,11 @@ contract MagicSpend is Ownable, IPaymaster {
             revert InvalidNonce(withdrawRequest.nonce);
         }
 
+        uint256 maxAllowed = address(this).balance / maxWithdrawDenominator;
+        if (withdrawRequest.asset == address(0) && withdrawRequest.amount > maxAllowed) {
+            revert WithdrawTooLarge(withdrawRequest.amount, maxAllowed);
+        }
+
         _nonceUsed[withdrawRequest.nonce][account] = true;
 
         // This is emitted ahead of fund transfer, but allows a consolidated code path
diff --git a/test/MagicSpend.t.sol b/test/MagicSpend.t.sol
index 308a09e..a2d2f07 100644
--- a/test/MagicSpend.t.sol
+++ b/test/MagicSpend.t.sol
@@ -10,7 +10,7 @@ contract MagicSpendTest is Test {
     address withdrawer = address(0xb0b);
     uint256 ownerPrivateKey = 0xa11ce;
     address owner = vm.addr(ownerPrivateKey);
-    MagicSpend magic = new MagicSpend(owner);
+    MagicSpend magic = new MagicSpend(owner, 1);
 
     // signature params
     address asset;
diff --git a/test/PostOp.t.sol b/test/PostOp.t.sol
index f50cb12..1e6d88d 100644
--- a/test/PostOp.t.sol
+++ b/test/PostOp.t.sol
@@ -32,7 +32,7 @@ contract PostOpTest is PaymasterMagicSpendBaseTest {
         vm.deal(address(magic), amount);
         (bytes memory context,) = magic.validatePaymasterUserOp(_getUserOp(), userOpHash, maxCost_);
         uint256 expectedBalance = 0;
-        vm.expectRevert();
+        vm.expectRevert(MagicSpend.UnexpectedPostOpRevertedMode.selector);
         magic.postOp(IPaymaster.PostOpMode.postOpReverted, context, actualCost);
         assertEq(withdrawer.balance, expectedBalance);
     }
diff --git a/test/Validate.t.sol b/test/Validate.t.sol
index d5078bb..f17b28b 100644
--- a/test/Validate.t.sol
+++ b/test/Validate.t.sol
@@ -6,6 +6,8 @@ import {Test, console2} from "forge-std/Test.sol";
 import "./MagicSpend.t.sol";
 
 abstract contract ValidateTest is MagicSpendTest {
+    address invoker;
+
     function test_recordsNonceUsed(uint256 nonce_) public {
         nonce = nonce_;
         assertFalse(magic.nonceUsed(withdrawer, nonce));
@@ -36,5 +38,31 @@ abstract contract ValidateTest is MagicSpendTest {
         _validateInvokingCall();
     }
 
+    function test_reverts_whenWithdrawExceedsMaxAllowed(
+        uint256 accountBalance,
+        uint256 withdraw,
+        uint256 maxWithdrawDenominator
+    ) public virtual {
+        vm.assume(maxWithdrawDenominator > 0);
+        accountBalance = bound(accountBalance, 0, type(uint256).max - 1);
+        withdraw = bound(withdraw, accountBalance / maxWithdrawDenominator + 1, type(uint256).max);
+        amount = withdraw;
+
+        vm.stopPrank();
+        vm.startPrank(owner);
+        magic.setMaxWithdrawDenominator(maxWithdrawDenominator);
+        magic.ownerWithdraw(address(0), address(1), address(magic).balance);
+        vm.stopPrank();
+
+        vm.deal(address(magic), accountBalance);
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                MagicSpend.WithdrawTooLarge.selector, withdraw, accountBalance / maxWithdrawDenominator
+            )
+        );
+        vm.startPrank(invoker);
+        _validateInvokingCall();
+    }
+
     function _validateInvokingCall() internal virtual;
 }
diff --git a/test/ValidatePaymasterUserOp.t.sol b/test/ValidatePaymasterUserOp.t.sol
index f6bb052..7614ecb 100644
--- a/test/ValidatePaymasterUserOp.t.sol
+++ b/test/ValidatePaymasterUserOp.t.sol
@@ -8,6 +8,7 @@ contract ValidatePaymasterUserOpTest is PaymasterMagicSpendBaseTest, ValidateTes
     function setUp() public override {
         super.setUp();
         vm.startPrank(magic.entryPoint());
+        invoker = magic.entryPoint();
     }
 
     function test_revertsIfMaxCostMoreThanRequested() public {
@@ -16,12 +17,6 @@ contract ValidatePaymasterUserOpTest is PaymasterMagicSpendBaseTest, ValidateTes
         magic.validatePaymasterUserOp(_getUserOp(), userOpHash, maxCost);
     }
 
-    function test_revertsIfWithdrawalExceedsBalance() public {
-        vm.deal(address(magic), 0);
-        vm.expectRevert(abi.encodeWithSelector(MagicSpend.InsufficientBalance.selector, amount, 0));
-        magic.validatePaymasterUserOp(_getUserOp(), userOpHash, maxCost);
-    }
-
     function test_returnsCorrectly() public {
         (bytes memory context, uint256 validationData) =
             magic.validatePaymasterUserOp(_getUserOp(), userOpHash, maxCost);
@@ -51,6 +46,15 @@ contract ValidatePaymasterUserOpTest is PaymasterMagicSpendBaseTest, ValidateTes
         super.test_emitsCorrectly(magic.entryPoint(), amount_, nonce_);
     }
 
+    function test_reverts_whenWithdrawExceedsMaxAllowed(
+        uint256 accountBalance,
+        uint256 withdraw,
+        uint256 maxWithdrawDenominator
+    ) public override {
+        maxCost = withdraw;
+        super.test_reverts_whenWithdrawExceedsMaxAllowed(accountBalance, withdraw, maxWithdrawDenominator);
+    }
+
     function _validateInvokingCall() internal override {
         magic.validatePaymasterUserOp(_getUserOp(), userOpHash, maxCost);
     }
diff --git a/test/Withdraw.t.sol b/test/Withdraw.t.sol
index 158ab16..7ac0fdb 100644
--- a/test/Withdraw.t.sol
+++ b/test/Withdraw.t.sol
@@ -11,6 +11,7 @@ contract WithdrawTest is MagicSpendTest, ValidateTest {
     function setUp() public override {
         super.setUp();
         vm.startPrank(withdrawer);
+        invoker = withdrawer;
     }
 
     function test_transfersETHSuccessfully(uint256 amount_) public {
diff --git a/test/echidna/FuzzSetup.sol b/test/echidna/FuzzSetup.sol
index 211c471..63d9c1c 100644
--- a/test/echidna/FuzzSetup.sol
+++ b/test/echidna/FuzzSetup.sol
@@ -22,7 +22,7 @@ contract FuzzSetup is FuzzBase {
     uint256 totalWithdrawn;
 
     constructor() payable {
-        magic = new MagicSpend(OWNER);
+        magic = new MagicSpend(OWNER, 1);
         address(magic).call{value: PAYMASTER_STARTING_BALANCE}("");
     }
 

From ecf7c15403b4870204aa964e66cdc278bac4e933 Mon Sep 17 00:00:00 2001
From: Wilson Cusack <wilson.cusack@coinbase.com>
Date: Thu, 4 Apr 2024 13:08:10 -0400
Subject: [PATCH 2/5] rename and add tests

---
 .gas-snapshot                      | 27 ++++++++++---------
 src/MagicSpend.sol                 | 42 ++++++++++++++++++------------
 test/OwnerWithdraw.t.sol           |  2 +-
 test/SetMaxWithdrawPercent.t.sol   | 26 ++++++++++++++++++
 test/Validate.t.sol                | 12 ++++-----
 test/ValidatePaymasterUserOp.t.sol |  4 +--
 6 files changed, 75 insertions(+), 38 deletions(-)
 create mode 100644 test/SetMaxWithdrawPercent.t.sol

diff --git a/.gas-snapshot b/.gas-snapshot
index d7cd5a7..8fb12d3 100644
--- a/.gas-snapshot
+++ b/.gas-snapshot
@@ -4,14 +4,17 @@ IsValidWithdrawalSignature:test_returnsTrueWithValidSignature() (gas: 51016)
 OwnerWithdrawTest:test_revertsIfNotOwner() (gas: 15305)
 OwnerWithdrawTest:test_transfersERC20Successfully(uint256) (runs: 256, μ: 97271, ~: 99711)
 OwnerWithdrawTest:test_transfersETHSuccessfully(uint256) (runs: 256, μ: 51418, ~: 53415)
-PostOpTest:test_RevertsIfPostOpFailed(uint256,uint256,uint256) (runs: 256, μ: 97167, ~: 99371)
-PostOpTest:test_entryPointAddStake(uint112,uint32) (runs: 256, μ: 56233, ~: 56233)
-PostOpTest:test_entryPointDeposit(uint112) (runs: 256, μ: 32516, ~: 32962)
-PostOpTest:test_entryPointUnlockStake() (gas: 54888)
-PostOpTest:test_entryPointWithdraw(uint112) (runs: 256, μ: 63200, ~: 65300)
-PostOpTest:test_entryPointWithdrawStake() (gas: 71803)
+PostOpTest:test_RevertsIfPostOpFailed(uint256,uint256,uint256) (runs: 256, μ: 97250, ~: 99371)
+PostOpTest:test_entryPointAddStake(uint112,uint32) (runs: 256, μ: 56278, ~: 56278)
+PostOpTest:test_entryPointDeposit(uint112) (runs: 256, μ: 32383, ~: 32940)
+PostOpTest:test_entryPointUnlockStake() (gas: 54933)
+PostOpTest:test_entryPointWithdraw(uint112) (runs: 256, μ: 63521, ~: 65321)
+PostOpTest:test_entryPointWithdrawStake() (gas: 71848)
 PostOpTest:test_paymasterPaysForOp() (gas: 210663)
-PostOpTest:test_transfersExcess(uint256,uint256,uint256,uint256) (runs: 256, μ: 112465, ~: 113488)
+PostOpTest:test_transfersExcess(uint256,uint256,uint256,uint256) (runs: 256, μ: 112468, ~: 113488)
+SetMaxWithdrawPercent:test_emitsCorrectly(uint256) (runs: 256, μ: 19851, ~: 20191)
+SetMaxWithdrawPercent:test_reverts_whenNotCalledByOwner() (gas: 12257)
+SetMaxWithdrawPercent:test_setsMaxWithdrawPercent(uint256) (runs: 256, μ: 19209, ~: 19549)
 Simulate:test() (gas: 12114)
 ValidatePaymasterUserOpTest:test_emitsCorrectly(address,uint256,uint256) (runs: 256, μ: 104847, ~: 106080)
 ValidatePaymasterUserOpTest:test_recordsNonceUsed(uint256) (runs: 256, μ: 112158, ~: 113247)
@@ -20,16 +23,16 @@ ValidatePaymasterUserOpTest:test_returnsCorrectly() (gas: 91185)
 ValidatePaymasterUserOpTest:test_revertsIfMaxCostMoreThanRequested() (gas: 39141)
 ValidatePaymasterUserOpTest:test_revertsIfNonceUsed() (gas: 103502)
 ValidatePaymasterUserOpTest:test_revertsIfWithdrawAssetNotZero() (gas: 55952)
-ValidatePaymasterUserOpTest:test_reverts_whenWithdrawExceedsMaxAllowed(uint256,uint256,uint256) (runs: 256, μ: 100811, ~: 100992)
+ValidatePaymasterUserOpTest:test_reverts_whenWithdrawExceedsMaxAllowed(uint256,uint256,uint256) (runs: 256, μ: 100800, ~: 100992)
 ValidateTest:test_receive() (gas: 14687)
-WithdrawGasExcess:test_RevertsIfNoExcess(uint256) (runs: 256, μ: 71133, ~: 71396)
+WithdrawGasExcess:test_RevertsIfNoExcess(uint256) (runs: 256, μ: 71122, ~: 71396)
 WithdrawGasExcess:test_transferExcess(uint256,uint256,uint256) (runs: 256, μ: 104279, ~: 104290)
-WithdrawTest:test_emitsCorrectly(address,uint256,uint256) (runs: 256, μ: 118413, ~: 121368)
-WithdrawTest:test_emitsCorrectlyERC20Withdraw(address,uint256,uint256) (runs: 256, μ: 162446, ~: 164773)
+WithdrawTest:test_emitsCorrectly(address,uint256,uint256) (runs: 256, μ: 118270, ~: 121368)
+WithdrawTest:test_emitsCorrectlyERC20Withdraw(address,uint256,uint256) (runs: 256, μ: 162408, ~: 164773)
 WithdrawTest:test_recordsNonceUsed(uint256) (runs: 256, μ: 111372, ~: 112461)
 WithdrawTest:test_revertsIfExpired(uint48,uint256) (runs: 256, μ: 61664, ~: 61785)
 WithdrawTest:test_revertsIfNonceUsed() (gas: 95844)
 WithdrawTest:test_revertsIfWrongSignature() (gas: 63830)
-WithdrawTest:test_reverts_whenWithdrawExceedsMaxAllowed(uint256,uint256,uint256) (runs: 256, μ: 85124, ~: 85096)
+WithdrawTest:test_reverts_whenWithdrawExceedsMaxAllowed(uint256,uint256,uint256) (runs: 256, μ: 85143, ~: 85096)
 WithdrawTest:test_transfersERC20Successfully(uint256) (runs: 256, μ: 140626, ~: 143066)
 WithdrawTest:test_transfersETHSuccessfully(uint256) (runs: 256, μ: 91692, ~: 93689)
\ No newline at end of file
diff --git a/src/MagicSpend.sol b/src/MagicSpend.sol
index aaebb53..a865fb1 100644
--- a/src/MagicSpend.sol
+++ b/src/MagicSpend.sol
@@ -20,7 +20,7 @@ contract MagicSpend is Ownable, IPaymaster {
     struct WithdrawRequest {
         /// @dev The signature associated with this withdraw request.
         bytes signature;
-        /// @dev The asset to withdraw. NOTE: Only ETH (associated with zero address) is supported for now.
+        /// @dev The asset to withdraw.
         address asset;
         /// @dev The requested amount to withdraw.
         uint256 amount;
@@ -30,9 +30,12 @@ contract MagicSpend is Ownable, IPaymaster {
         uint48 expiry;
     }
 
-    /// @notice The maximum percentage of address.balance a single withdraw.
+    /// @notice The maximum percentage of address.balance a single withdraw can use.
+    ///
+    /// @dev Only applies to native asset withdraws.
     /// @dev Helps prevent withdraws in the same transaction leading to reverts and hurting paymaster reputation.
-    uint256 public maxWithdrawDenominator;
+    /// @dev Percent in whole units; 20 = 1/20 = 20%
+    uint256 public maxWithdrawPercent;
 
     /// @notice Track the ETH available to be withdrawn per user.
     mapping(address user => uint256 amount) internal _withdrawableETH;
@@ -48,10 +51,10 @@ contract MagicSpend is Ownable, IPaymaster {
     /// @param nonce   The request nonce.
     event MagicSpendWithdrawal(address indexed account, address indexed asset, uint256 amount, uint256 nonce);
 
-    /// @notice Emitted when the `maxWithdrawDenominator` is set.
+    /// @notice Emitted when the `maxWithdrawPercent` is set.
     ///
-    /// @param newDenominator The new maxWithdrawDenominator value.
-    event MaxWithdrawDenominatorSet(uint256 newDenominator);
+    /// @param newPercent The new maxWithdrawPercent value.
+    event MaxWithdrawPercentSet(uint256 newPercent);
 
     /// @notice Thrown when the withdraw request signature is invalid.
     ///
@@ -81,7 +84,7 @@ contract MagicSpend is Ownable, IPaymaster {
     /// @param asset The requested asset.
     error UnsupportedPaymasterAsset(address asset);
 
-    /// @notice Thrown if WithdrawRequest.amount exceeds address(this).balance / maxWithdrawDenominator.
+    /// @notice Thrown if WithdrawRequest.amount exceeds address(this).balance / maxWithdrawPercent.
     ///
     /// @param requestedAmount The requested amount excluding gas.
     /// @param maxAllowed      The current max allowed withdraw.
@@ -106,9 +109,9 @@ contract MagicSpend is Ownable, IPaymaster {
     /// @notice Deploy the contract and set its initial owner.
     ///
     /// @param owner_ The initial owner of this contract.
-    constructor(address owner_, uint256 maxWithdrawDenominator_) {
+    constructor(address owner_, uint256 maxWithdrawPercent_) {
         Ownable._initializeOwner(owner_);
-        _setMaxWithdrawDenominator(maxWithdrawDenominator_);
+        _setMaxWithdrawPercent(maxWithdrawPercent_);
     }
 
     /// @notice Receive function allowing ETH to be deposited in this contract.
@@ -147,7 +150,7 @@ contract MagicSpend is Ownable, IPaymaster {
         onlyEntryPoint
     {
         // `PostOpMode.postOpReverted` should never happen.
-        // The flow here can only revert if there are > maxWithdrawDenominator
+        // The flow here can only revert if there are > maxWithdrawPercent
         // withdraws in the same transaction, which should be highly unlikely.
         // If the ETH transfer fails, the entire bundle will revert due an issue in the EntryPoint
         // https://github.com/eth-infinitism/account-abstraction/pull/293
@@ -255,8 +258,15 @@ contract MagicSpend is Ownable, IPaymaster {
         IEntryPoint(entryPoint()).withdrawStake(to);
     }
 
-    function setMaxWithdrawDenominator(uint256 newDenominator) external onlyOwner {
-        _setMaxWithdrawDenominator(newDenominator);
+    /// @notice sets maxWithdrawPercent
+    ///
+    /// @dev Reverts if not called by the owner of the contract.
+    ///
+    /// @param newPercent The new value for maxWithdrawPercent
+    /// Percent expressed in whole units, e.g. 20 means no single withdraw
+    /// can exceed 20% of address.balance
+    function setMaxWithdrawPercent(uint256 newPercent) external onlyOwner {
+        _setMaxWithdrawPercent(newPercent);
     }
 
     /// @notice Returns whether the `withdrawRequest` signature is valid for the given `account`.
@@ -315,10 +325,10 @@ contract MagicSpend is Ownable, IPaymaster {
         return 0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789;
     }
 
-    function _setMaxWithdrawDenominator(uint256 newDenominator) internal {
-        maxWithdrawDenominator = newDenominator;
+    function _setMaxWithdrawPercent(uint256 newPercent) internal {
+        maxWithdrawPercent = newPercent;
 
-        emit MaxWithdrawDenominatorSet(newDenominator);
+        emit MaxWithdrawPercentSet(newPercent);
     }
 
     /// @notice Validate the `withdrawRequest` against the given `account`.
@@ -333,7 +343,7 @@ contract MagicSpend is Ownable, IPaymaster {
             revert InvalidNonce(withdrawRequest.nonce);
         }
 
-        uint256 maxAllowed = address(this).balance / maxWithdrawDenominator;
+        uint256 maxAllowed = address(this).balance / maxWithdrawPercent;
         if (withdrawRequest.asset == address(0) && withdrawRequest.amount > maxAllowed) {
             revert WithdrawTooLarge(withdrawRequest.amount, maxAllowed);
         }
diff --git a/test/OwnerWithdraw.t.sol b/test/OwnerWithdraw.t.sol
index 32026ee..3889377 100644
--- a/test/OwnerWithdraw.t.sol
+++ b/test/OwnerWithdraw.t.sol
@@ -2,7 +2,7 @@
 pragma solidity >=0.8.21;
 
 import {Ownable} from "./MagicSpend.t.sol";
-import {MagicSpendTest} from "./Validate.t.sol";
+import {MagicSpendTest} from "./MagicSpend.t.sol";
 import {MockERC20} from "solady/test/utils/mocks/MockERC20.sol";
 
 contract OwnerWithdrawTest is MagicSpendTest {
diff --git a/test/SetMaxWithdrawPercent.t.sol b/test/SetMaxWithdrawPercent.t.sol
new file mode 100644
index 0000000..42b32c0
--- /dev/null
+++ b/test/SetMaxWithdrawPercent.t.sol
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.23;
+
+import "./MagicSpend.t.sol";
+import {Ownable} from "./MagicSpend.t.sol";
+
+contract SetMaxWithdrawPercent is MagicSpendTest {
+    function test_reverts_whenNotCalledByOwner() public {
+        vm.prank(makeAddr("fake"));
+        vm.expectRevert(Ownable.Unauthorized.selector);
+        magic.setMaxWithdrawPercent(20);
+    }
+
+    function test_setsMaxWithdrawPercent(uint256 newPercent) public {
+        vm.prank(owner);
+        magic.setMaxWithdrawPercent(newPercent);
+        assertEq(magic.maxWithdrawPercent(), newPercent);
+    }
+
+    function test_emitsCorrectly(uint256 newPercent) public {
+        vm.expectEmit(false, false, false, true);
+        emit MagicSpend.MaxWithdrawPercentSet(newPercent);
+        vm.prank(owner);
+        magic.setMaxWithdrawPercent(newPercent);
+    }
+}
diff --git a/test/Validate.t.sol b/test/Validate.t.sol
index f17b28b..ccf8fb8 100644
--- a/test/Validate.t.sol
+++ b/test/Validate.t.sol
@@ -41,24 +41,22 @@ abstract contract ValidateTest is MagicSpendTest {
     function test_reverts_whenWithdrawExceedsMaxAllowed(
         uint256 accountBalance,
         uint256 withdraw,
-        uint256 maxWithdrawDenominator
+        uint256 maxWithdrawPercent
     ) public virtual {
-        vm.assume(maxWithdrawDenominator > 0);
+        vm.assume(maxWithdrawPercent > 0);
         accountBalance = bound(accountBalance, 0, type(uint256).max - 1);
-        withdraw = bound(withdraw, accountBalance / maxWithdrawDenominator + 1, type(uint256).max);
+        withdraw = bound(withdraw, accountBalance / maxWithdrawPercent + 1, type(uint256).max);
         amount = withdraw;
 
         vm.stopPrank();
         vm.startPrank(owner);
-        magic.setMaxWithdrawDenominator(maxWithdrawDenominator);
+        magic.setMaxWithdrawPercent(maxWithdrawPercent);
         magic.ownerWithdraw(address(0), address(1), address(magic).balance);
         vm.stopPrank();
 
         vm.deal(address(magic), accountBalance);
         vm.expectRevert(
-            abi.encodeWithSelector(
-                MagicSpend.WithdrawTooLarge.selector, withdraw, accountBalance / maxWithdrawDenominator
-            )
+            abi.encodeWithSelector(MagicSpend.WithdrawTooLarge.selector, withdraw, accountBalance / maxWithdrawPercent)
         );
         vm.startPrank(invoker);
         _validateInvokingCall();
diff --git a/test/ValidatePaymasterUserOp.t.sol b/test/ValidatePaymasterUserOp.t.sol
index 7614ecb..dad731c 100644
--- a/test/ValidatePaymasterUserOp.t.sol
+++ b/test/ValidatePaymasterUserOp.t.sol
@@ -49,10 +49,10 @@ contract ValidatePaymasterUserOpTest is PaymasterMagicSpendBaseTest, ValidateTes
     function test_reverts_whenWithdrawExceedsMaxAllowed(
         uint256 accountBalance,
         uint256 withdraw,
-        uint256 maxWithdrawDenominator
+        uint256 maxWithdrawPercent
     ) public override {
         maxCost = withdraw;
-        super.test_reverts_whenWithdrawExceedsMaxAllowed(accountBalance, withdraw, maxWithdrawDenominator);
+        super.test_reverts_whenWithdrawExceedsMaxAllowed(accountBalance, withdraw, maxWithdrawPercent);
     }
 
     function _validateInvokingCall() internal override {

From e5efda236c94488a2ed3338e5d01428246bcc3ea Mon Sep 17 00:00:00 2001
From: Wilson Cusack <wilson.cusack@coinbase.com>
Date: Thu, 4 Apr 2024 13:59:16 -0400
Subject: [PATCH 3/5] clean up natspec

---
 .gas-snapshot      | 22 +++++++++++-----------
 src/MagicSpend.sol |  7 +++----
 2 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/.gas-snapshot b/.gas-snapshot
index 8fb12d3..21ef4af 100644
--- a/.gas-snapshot
+++ b/.gas-snapshot
@@ -4,14 +4,14 @@ IsValidWithdrawalSignature:test_returnsTrueWithValidSignature() (gas: 51016)
 OwnerWithdrawTest:test_revertsIfNotOwner() (gas: 15305)
 OwnerWithdrawTest:test_transfersERC20Successfully(uint256) (runs: 256, μ: 97271, ~: 99711)
 OwnerWithdrawTest:test_transfersETHSuccessfully(uint256) (runs: 256, μ: 51418, ~: 53415)
-PostOpTest:test_RevertsIfPostOpFailed(uint256,uint256,uint256) (runs: 256, μ: 97250, ~: 99371)
+PostOpTest:test_RevertsIfPostOpFailed(uint256,uint256,uint256) (runs: 256, μ: 97229, ~: 99371)
 PostOpTest:test_entryPointAddStake(uint112,uint32) (runs: 256, μ: 56278, ~: 56278)
-PostOpTest:test_entryPointDeposit(uint112) (runs: 256, μ: 32383, ~: 32940)
+PostOpTest:test_entryPointDeposit(uint112) (runs: 256, μ: 32457, ~: 32940)
 PostOpTest:test_entryPointUnlockStake() (gas: 54933)
-PostOpTest:test_entryPointWithdraw(uint112) (runs: 256, μ: 63521, ~: 65321)
+PostOpTest:test_entryPointWithdraw(uint112) (runs: 256, μ: 63371, ~: 65321)
 PostOpTest:test_entryPointWithdrawStake() (gas: 71848)
 PostOpTest:test_paymasterPaysForOp() (gas: 210663)
-PostOpTest:test_transfersExcess(uint256,uint256,uint256,uint256) (runs: 256, μ: 112468, ~: 113488)
+PostOpTest:test_transfersExcess(uint256,uint256,uint256,uint256) (runs: 256, μ: 112452, ~: 113488)
 SetMaxWithdrawPercent:test_emitsCorrectly(uint256) (runs: 256, μ: 19851, ~: 20191)
 SetMaxWithdrawPercent:test_reverts_whenNotCalledByOwner() (gas: 12257)
 SetMaxWithdrawPercent:test_setsMaxWithdrawPercent(uint256) (runs: 256, μ: 19209, ~: 19549)
@@ -23,16 +23,16 @@ ValidatePaymasterUserOpTest:test_returnsCorrectly() (gas: 91185)
 ValidatePaymasterUserOpTest:test_revertsIfMaxCostMoreThanRequested() (gas: 39141)
 ValidatePaymasterUserOpTest:test_revertsIfNonceUsed() (gas: 103502)
 ValidatePaymasterUserOpTest:test_revertsIfWithdrawAssetNotZero() (gas: 55952)
-ValidatePaymasterUserOpTest:test_reverts_whenWithdrawExceedsMaxAllowed(uint256,uint256,uint256) (runs: 256, μ: 100800, ~: 100992)
+ValidatePaymasterUserOpTest:test_reverts_whenWithdrawExceedsMaxAllowed(uint256,uint256,uint256) (runs: 256, μ: 100793, ~: 100992)
 ValidateTest:test_receive() (gas: 14687)
-WithdrawGasExcess:test_RevertsIfNoExcess(uint256) (runs: 256, μ: 71122, ~: 71396)
-WithdrawGasExcess:test_transferExcess(uint256,uint256,uint256) (runs: 256, μ: 104279, ~: 104290)
-WithdrawTest:test_emitsCorrectly(address,uint256,uint256) (runs: 256, μ: 118270, ~: 121368)
-WithdrawTest:test_emitsCorrectlyERC20Withdraw(address,uint256,uint256) (runs: 256, μ: 162408, ~: 164773)
+WithdrawGasExcess:test_RevertsIfNoExcess(uint256) (runs: 256, μ: 71133, ~: 71396)
+WithdrawGasExcess:test_transferExcess(uint256,uint256,uint256) (runs: 256, μ: 104290, ~: 104290)
+WithdrawTest:test_emitsCorrectly(address,uint256,uint256) (runs: 256, μ: 118193, ~: 121368)
+WithdrawTest:test_emitsCorrectlyERC20Withdraw(address,uint256,uint256) (runs: 256, μ: 162446, ~: 164773)
 WithdrawTest:test_recordsNonceUsed(uint256) (runs: 256, μ: 111372, ~: 112461)
-WithdrawTest:test_revertsIfExpired(uint48,uint256) (runs: 256, μ: 61664, ~: 61785)
+WithdrawTest:test_revertsIfExpired(uint48,uint256) (runs: 256, μ: 61675, ~: 61785)
 WithdrawTest:test_revertsIfNonceUsed() (gas: 95844)
 WithdrawTest:test_revertsIfWrongSignature() (gas: 63830)
-WithdrawTest:test_reverts_whenWithdrawExceedsMaxAllowed(uint256,uint256,uint256) (runs: 256, μ: 85143, ~: 85096)
+WithdrawTest:test_reverts_whenWithdrawExceedsMaxAllowed(uint256,uint256,uint256) (runs: 256, μ: 85154, ~: 85096)
 WithdrawTest:test_transfersERC20Successfully(uint256) (runs: 256, μ: 140626, ~: 143066)
 WithdrawTest:test_transfersETHSuccessfully(uint256) (runs: 256, μ: 91692, ~: 93689)
\ No newline at end of file
diff --git a/src/MagicSpend.sol b/src/MagicSpend.sol
index a865fb1..4e6fa50 100644
--- a/src/MagicSpend.sol
+++ b/src/MagicSpend.sol
@@ -34,7 +34,8 @@ contract MagicSpend is Ownable, IPaymaster {
     ///
     /// @dev Only applies to native asset withdraws.
     /// @dev Helps prevent withdraws in the same transaction leading to reverts and hurting paymaster reputation.
-    /// @dev Percent in whole units; 20 = 1/20 = 20%
+    /// @dev Percent expressed in whole units, e.g. 20 means no single withdraw
+    /// can exceed 20% of address.balance
     uint256 public maxWithdrawPercent;
 
     /// @notice Track the ETH available to be withdrawn per user.
@@ -262,9 +263,7 @@ contract MagicSpend is Ownable, IPaymaster {
     ///
     /// @dev Reverts if not called by the owner of the contract.
     ///
-    /// @param newPercent The new value for maxWithdrawPercent
-    /// Percent expressed in whole units, e.g. 20 means no single withdraw
-    /// can exceed 20% of address.balance
+    /// @param newPercent The new value for maxWithdrawPercent.
     function setMaxWithdrawPercent(uint256 newPercent) external onlyOwner {
         _setMaxWithdrawPercent(newPercent);
     }

From fc38b23379eeee9c5babf0289e1a634935e38ee1 Mon Sep 17 00:00:00 2001
From: Wilson Cusack <wilson.cusack@coinbase.com>
Date: Thu, 4 Apr 2024 14:43:01 -0400
Subject: [PATCH 4/5] renamed

---
 .gas-snapshot                        | 30 ++++++++--------
 src/MagicSpend.sol                   | 53 ++++++++++++++--------------
 test/SetMaxWithdrawDenominator.t.sol | 26 ++++++++++++++
 test/SetMaxWithdrawPercent.t.sol     | 26 --------------
 test/Validate.t.sol                  | 12 ++++---
 test/ValidatePaymasterUserOp.t.sol   |  4 +--
 6 files changed, 76 insertions(+), 75 deletions(-)
 create mode 100644 test/SetMaxWithdrawDenominator.t.sol
 delete mode 100644 test/SetMaxWithdrawPercent.t.sol

diff --git a/.gas-snapshot b/.gas-snapshot
index 21ef4af..301812d 100644
--- a/.gas-snapshot
+++ b/.gas-snapshot
@@ -4,17 +4,17 @@ IsValidWithdrawalSignature:test_returnsTrueWithValidSignature() (gas: 51016)
 OwnerWithdrawTest:test_revertsIfNotOwner() (gas: 15305)
 OwnerWithdrawTest:test_transfersERC20Successfully(uint256) (runs: 256, μ: 97271, ~: 99711)
 OwnerWithdrawTest:test_transfersETHSuccessfully(uint256) (runs: 256, μ: 51418, ~: 53415)
-PostOpTest:test_RevertsIfPostOpFailed(uint256,uint256,uint256) (runs: 256, μ: 97229, ~: 99371)
-PostOpTest:test_entryPointAddStake(uint112,uint32) (runs: 256, μ: 56278, ~: 56278)
-PostOpTest:test_entryPointDeposit(uint112) (runs: 256, μ: 32457, ~: 32940)
-PostOpTest:test_entryPointUnlockStake() (gas: 54933)
-PostOpTest:test_entryPointWithdraw(uint112) (runs: 256, μ: 63371, ~: 65321)
-PostOpTest:test_entryPointWithdrawStake() (gas: 71848)
+PostOpTest:test_RevertsIfPostOpFailed(uint256,uint256,uint256) (runs: 256, μ: 97259, ~: 99371)
+PostOpTest:test_entryPointAddStake(uint112,uint32) (runs: 256, μ: 56233, ~: 56233)
+PostOpTest:test_entryPointDeposit(uint112) (runs: 256, μ: 32479, ~: 32962)
+PostOpTest:test_entryPointUnlockStake() (gas: 54888)
+PostOpTest:test_entryPointWithdraw(uint112) (runs: 256, μ: 63050, ~: 65300)
+PostOpTest:test_entryPointWithdrawStake() (gas: 71803)
 PostOpTest:test_paymasterPaysForOp() (gas: 210663)
-PostOpTest:test_transfersExcess(uint256,uint256,uint256,uint256) (runs: 256, μ: 112452, ~: 113488)
-SetMaxWithdrawPercent:test_emitsCorrectly(uint256) (runs: 256, μ: 19851, ~: 20191)
-SetMaxWithdrawPercent:test_reverts_whenNotCalledByOwner() (gas: 12257)
-SetMaxWithdrawPercent:test_setsMaxWithdrawPercent(uint256) (runs: 256, μ: 19209, ~: 19549)
+PostOpTest:test_transfersExcess(uint256,uint256,uint256,uint256) (runs: 256, μ: 112442, ~: 113477)
+SetMaxWithdrawDenominator:test_emitsCorrectly(uint256) (runs: 256, μ: 19851, ~: 20191)
+SetMaxWithdrawDenominator:test_reverts_whenNotCalledByOwner() (gas: 12257)
+SetMaxWithdrawDenominator:test_setsMaxWithdrawPercent(uint256) (runs: 256, μ: 19209, ~: 19549)
 Simulate:test() (gas: 12114)
 ValidatePaymasterUserOpTest:test_emitsCorrectly(address,uint256,uint256) (runs: 256, μ: 104847, ~: 106080)
 ValidatePaymasterUserOpTest:test_recordsNonceUsed(uint256) (runs: 256, μ: 112158, ~: 113247)
@@ -23,16 +23,16 @@ ValidatePaymasterUserOpTest:test_returnsCorrectly() (gas: 91185)
 ValidatePaymasterUserOpTest:test_revertsIfMaxCostMoreThanRequested() (gas: 39141)
 ValidatePaymasterUserOpTest:test_revertsIfNonceUsed() (gas: 103502)
 ValidatePaymasterUserOpTest:test_revertsIfWithdrawAssetNotZero() (gas: 55952)
-ValidatePaymasterUserOpTest:test_reverts_whenWithdrawExceedsMaxAllowed(uint256,uint256,uint256) (runs: 256, μ: 100793, ~: 100992)
+ValidatePaymasterUserOpTest:test_reverts_whenWithdrawExceedsMaxAllowed(uint256,uint256,uint256) (runs: 256, μ: 100806, ~: 100992)
 ValidateTest:test_receive() (gas: 14687)
 WithdrawGasExcess:test_RevertsIfNoExcess(uint256) (runs: 256, μ: 71133, ~: 71396)
 WithdrawGasExcess:test_transferExcess(uint256,uint256,uint256) (runs: 256, μ: 104290, ~: 104290)
-WithdrawTest:test_emitsCorrectly(address,uint256,uint256) (runs: 256, μ: 118193, ~: 121368)
-WithdrawTest:test_emitsCorrectlyERC20Withdraw(address,uint256,uint256) (runs: 256, μ: 162446, ~: 164773)
+WithdrawTest:test_emitsCorrectly(address,uint256,uint256) (runs: 256, μ: 118270, ~: 121368)
+WithdrawTest:test_emitsCorrectlyERC20Withdraw(address,uint256,uint256) (runs: 256, μ: 162427, ~: 164773)
 WithdrawTest:test_recordsNonceUsed(uint256) (runs: 256, μ: 111372, ~: 112461)
-WithdrawTest:test_revertsIfExpired(uint48,uint256) (runs: 256, μ: 61675, ~: 61785)
+WithdrawTest:test_revertsIfExpired(uint48,uint256) (runs: 256, μ: 61664, ~: 61785)
 WithdrawTest:test_revertsIfNonceUsed() (gas: 95844)
 WithdrawTest:test_revertsIfWrongSignature() (gas: 63830)
-WithdrawTest:test_reverts_whenWithdrawExceedsMaxAllowed(uint256,uint256,uint256) (runs: 256, μ: 85154, ~: 85096)
+WithdrawTest:test_reverts_whenWithdrawExceedsMaxAllowed(uint256,uint256,uint256) (runs: 256, μ: 85119, ~: 85096)
 WithdrawTest:test_transfersERC20Successfully(uint256) (runs: 256, μ: 140626, ~: 143066)
 WithdrawTest:test_transfersETHSuccessfully(uint256) (runs: 256, μ: 91692, ~: 93689)
\ No newline at end of file
diff --git a/src/MagicSpend.sol b/src/MagicSpend.sol
index 4e6fa50..6073702 100644
--- a/src/MagicSpend.sol
+++ b/src/MagicSpend.sol
@@ -30,16 +30,14 @@ contract MagicSpend is Ownable, IPaymaster {
         uint48 expiry;
     }
 
-    /// @notice The maximum percentage of address.balance a single withdraw can use.
+    /// @notice address.balance divided by maxWithdrawDenominator expresses the max WithdrawRequest.amount
+    /// allowed for a native asset withdraw.
     ///
-    /// @dev Only applies to native asset withdraws.
     /// @dev Helps prevent withdraws in the same transaction leading to reverts and hurting paymaster reputation.
-    /// @dev Percent expressed in whole units, e.g. 20 means no single withdraw
-    /// can exceed 20% of address.balance
-    uint256 public maxWithdrawPercent;
+    uint256 public maxWithdrawDenominator;
 
-    /// @notice Track the ETH available to be withdrawn per user.
-    mapping(address user => uint256 amount) internal _withdrawableETH;
+    /// @notice Track the amount of native asset available to be withdrawn per user.
+    mapping(address user => uint256 amount) internal _withdrawable;
 
     /// @dev Mappings keeping track of already used nonces per user to prevent replays of withdraw requests.
     mapping(uint256 nonce => mapping(address user => bool used)) internal _nonceUsed;
@@ -52,10 +50,10 @@ contract MagicSpend is Ownable, IPaymaster {
     /// @param nonce   The request nonce.
     event MagicSpendWithdrawal(address indexed account, address indexed asset, uint256 amount, uint256 nonce);
 
-    /// @notice Emitted when the `maxWithdrawPercent` is set.
+    /// @notice Emitted when the `maxWithdrawDenominator` is set.
     ///
-    /// @param newPercent The new maxWithdrawPercent value.
-    event MaxWithdrawPercentSet(uint256 newPercent);
+    /// @param newDenominator The new maxWithdrawDenominator value.
+    event MaxWithdrawDenominatorSet(uint256 newDenominator);
 
     /// @notice Thrown when the withdraw request signature is invalid.
     ///
@@ -85,7 +83,7 @@ contract MagicSpend is Ownable, IPaymaster {
     /// @param asset The requested asset.
     error UnsupportedPaymasterAsset(address asset);
 
-    /// @notice Thrown if WithdrawRequest.amount exceeds address(this).balance / maxWithdrawPercent.
+    /// @notice Thrown if WithdrawRequest.amount exceeds address(this).balance / maxWithdrawDenominator.
     ///
     /// @param requestedAmount The requested amount excluding gas.
     /// @param maxAllowed      The current max allowed withdraw.
@@ -110,9 +108,10 @@ contract MagicSpend is Ownable, IPaymaster {
     /// @notice Deploy the contract and set its initial owner.
     ///
     /// @param owner_ The initial owner of this contract.
-    constructor(address owner_, uint256 maxWithdrawPercent_) {
+    /// @param maxWithdrawDenominator_ The initial maxWithdrawDenominator.
+    constructor(address owner_, uint256 maxWithdrawDenominator_) {
         Ownable._initializeOwner(owner_);
-        _setMaxWithdrawPercent(maxWithdrawPercent_);
+        _setMaxWithdrawDenominator(maxWithdrawDenominator_);
     }
 
     /// @notice Receive function allowing ETH to be deposited in this contract.
@@ -141,7 +140,7 @@ contract MagicSpend is Ownable, IPaymaster {
         validationData = (sigFailed ? 1 : 0) | (uint256(withdrawRequest.expiry) << 160);
 
         // NOTE: Do not include the gas part in withdrawable funds as it will be handled in `postOp()`.
-        _withdrawableETH[userOp.sender] += withdrawAmount - maxCost;
+        _withdrawable[userOp.sender] += withdrawAmount - maxCost;
         context = abi.encode(maxCost, userOp.sender);
     }
 
@@ -151,7 +150,7 @@ contract MagicSpend is Ownable, IPaymaster {
         onlyEntryPoint
     {
         // `PostOpMode.postOpReverted` should never happen.
-        // The flow here can only revert if there are > maxWithdrawPercent
+        // The flow here can only revert if there are > maxWithdrawDenominator
         // withdraws in the same transaction, which should be highly unlikely.
         // If the ETH transfer fails, the entire bundle will revert due an issue in the EntryPoint
         // https://github.com/eth-infinitism/account-abstraction/pull/293
@@ -163,10 +162,10 @@ contract MagicSpend is Ownable, IPaymaster {
 
         // Compute the total remaining funds available for the user accout.
         // NOTE: Take into account the user operation gas that was not consumed.
-        uint256 withdrawable = _withdrawableETH[account] + (maxGasCost - actualGasCost);
+        uint256 withdrawable = _withdrawable[account] + (maxGasCost - actualGasCost);
 
         // Send the all remaining funds to the user accout.
-        delete _withdrawableETH[account];
+        delete _withdrawable[account];
         if (withdrawable > 0) {
             SafeTransferLib.forceSafeTransferETH(account, withdrawable, SafeTransferLib.GAS_STIPEND_NO_STORAGE_WRITES);
         }
@@ -177,11 +176,11 @@ contract MagicSpend is Ownable, IPaymaster {
     /// @dev Can be called back during the `UserOperation` execution to sponsor funds for non-gas related
     ///      use cases (e.g., swap or mint).
     function withdrawGasExcess() external {
-        uint256 amount = _withdrawableETH[msg.sender];
+        uint256 amount = _withdrawable[msg.sender];
         // we could allow 0 value transfers, but prefer to be explicit
         if (amount == 0) revert NoExcess();
 
-        delete _withdrawableETH[msg.sender];
+        delete _withdrawable[msg.sender];
         _withdraw(address(0), msg.sender, amount);
     }
 
@@ -259,13 +258,13 @@ contract MagicSpend is Ownable, IPaymaster {
         IEntryPoint(entryPoint()).withdrawStake(to);
     }
 
-    /// @notice sets maxWithdrawPercent
+    /// @notice Sets maxWithdrawDenominator.
     ///
     /// @dev Reverts if not called by the owner of the contract.
     ///
-    /// @param newPercent The new value for maxWithdrawPercent.
-    function setMaxWithdrawPercent(uint256 newPercent) external onlyOwner {
-        _setMaxWithdrawPercent(newPercent);
+    /// @param newDenominator The new value for maxWithdrawDenominator.
+    function setMaxWithdrawDenominator(uint256 newDenominator) external onlyOwner {
+        _setMaxWithdrawDenominator(newDenominator);
     }
 
     /// @notice Returns whether the `withdrawRequest` signature is valid for the given `account`.
@@ -324,10 +323,10 @@ contract MagicSpend is Ownable, IPaymaster {
         return 0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789;
     }
 
-    function _setMaxWithdrawPercent(uint256 newPercent) internal {
-        maxWithdrawPercent = newPercent;
+    function _setMaxWithdrawDenominator(uint256 newDenominator) internal {
+        maxWithdrawDenominator = newDenominator;
 
-        emit MaxWithdrawPercentSet(newPercent);
+        emit MaxWithdrawDenominatorSet(newDenominator);
     }
 
     /// @notice Validate the `withdrawRequest` against the given `account`.
@@ -342,7 +341,7 @@ contract MagicSpend is Ownable, IPaymaster {
             revert InvalidNonce(withdrawRequest.nonce);
         }
 
-        uint256 maxAllowed = address(this).balance / maxWithdrawPercent;
+        uint256 maxAllowed = address(this).balance / maxWithdrawDenominator;
         if (withdrawRequest.asset == address(0) && withdrawRequest.amount > maxAllowed) {
             revert WithdrawTooLarge(withdrawRequest.amount, maxAllowed);
         }
diff --git a/test/SetMaxWithdrawDenominator.t.sol b/test/SetMaxWithdrawDenominator.t.sol
new file mode 100644
index 0000000..5026349
--- /dev/null
+++ b/test/SetMaxWithdrawDenominator.t.sol
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.23;
+
+import "./MagicSpend.t.sol";
+import {Ownable} from "./MagicSpend.t.sol";
+
+contract SetMaxWithdrawDenominator is MagicSpendTest {
+    function test_reverts_whenNotCalledByOwner() public {
+        vm.prank(makeAddr("fake"));
+        vm.expectRevert(Ownable.Unauthorized.selector);
+        magic.setMaxWithdrawDenominator(20);
+    }
+
+    function test_setsMaxWithdrawPercent(uint256 newDenominator) public {
+        vm.prank(owner);
+        magic.setMaxWithdrawDenominator(newDenominator);
+        assertEq(magic.maxWithdrawDenominator(), newDenominator);
+    }
+
+    function test_emitsCorrectly(uint256 newDenominator) public {
+        vm.expectEmit(false, false, false, true);
+        emit MagicSpend.MaxWithdrawDenominatorSet(newDenominator);
+        vm.prank(owner);
+        magic.setMaxWithdrawDenominator(newDenominator);
+    }
+}
diff --git a/test/SetMaxWithdrawPercent.t.sol b/test/SetMaxWithdrawPercent.t.sol
deleted file mode 100644
index 42b32c0..0000000
--- a/test/SetMaxWithdrawPercent.t.sol
+++ /dev/null
@@ -1,26 +0,0 @@
-// SPDX-License-Identifier: MIT
-pragma solidity ^0.8.23;
-
-import "./MagicSpend.t.sol";
-import {Ownable} from "./MagicSpend.t.sol";
-
-contract SetMaxWithdrawPercent is MagicSpendTest {
-    function test_reverts_whenNotCalledByOwner() public {
-        vm.prank(makeAddr("fake"));
-        vm.expectRevert(Ownable.Unauthorized.selector);
-        magic.setMaxWithdrawPercent(20);
-    }
-
-    function test_setsMaxWithdrawPercent(uint256 newPercent) public {
-        vm.prank(owner);
-        magic.setMaxWithdrawPercent(newPercent);
-        assertEq(magic.maxWithdrawPercent(), newPercent);
-    }
-
-    function test_emitsCorrectly(uint256 newPercent) public {
-        vm.expectEmit(false, false, false, true);
-        emit MagicSpend.MaxWithdrawPercentSet(newPercent);
-        vm.prank(owner);
-        magic.setMaxWithdrawPercent(newPercent);
-    }
-}
diff --git a/test/Validate.t.sol b/test/Validate.t.sol
index ccf8fb8..f17b28b 100644
--- a/test/Validate.t.sol
+++ b/test/Validate.t.sol
@@ -41,22 +41,24 @@ abstract contract ValidateTest is MagicSpendTest {
     function test_reverts_whenWithdrawExceedsMaxAllowed(
         uint256 accountBalance,
         uint256 withdraw,
-        uint256 maxWithdrawPercent
+        uint256 maxWithdrawDenominator
     ) public virtual {
-        vm.assume(maxWithdrawPercent > 0);
+        vm.assume(maxWithdrawDenominator > 0);
         accountBalance = bound(accountBalance, 0, type(uint256).max - 1);
-        withdraw = bound(withdraw, accountBalance / maxWithdrawPercent + 1, type(uint256).max);
+        withdraw = bound(withdraw, accountBalance / maxWithdrawDenominator + 1, type(uint256).max);
         amount = withdraw;
 
         vm.stopPrank();
         vm.startPrank(owner);
-        magic.setMaxWithdrawPercent(maxWithdrawPercent);
+        magic.setMaxWithdrawDenominator(maxWithdrawDenominator);
         magic.ownerWithdraw(address(0), address(1), address(magic).balance);
         vm.stopPrank();
 
         vm.deal(address(magic), accountBalance);
         vm.expectRevert(
-            abi.encodeWithSelector(MagicSpend.WithdrawTooLarge.selector, withdraw, accountBalance / maxWithdrawPercent)
+            abi.encodeWithSelector(
+                MagicSpend.WithdrawTooLarge.selector, withdraw, accountBalance / maxWithdrawDenominator
+            )
         );
         vm.startPrank(invoker);
         _validateInvokingCall();
diff --git a/test/ValidatePaymasterUserOp.t.sol b/test/ValidatePaymasterUserOp.t.sol
index dad731c..bd52349 100644
--- a/test/ValidatePaymasterUserOp.t.sol
+++ b/test/ValidatePaymasterUserOp.t.sol
@@ -49,10 +49,10 @@ contract ValidatePaymasterUserOpTest is PaymasterMagicSpendBaseTest, ValidateTes
     function test_reverts_whenWithdrawExceedsMaxAllowed(
         uint256 accountBalance,
         uint256 withdraw,
-        uint256 maxWithdrawPercent
+        uint256 MaxWithdrawDenominator
     ) public override {
         maxCost = withdraw;
-        super.test_reverts_whenWithdrawExceedsMaxAllowed(accountBalance, withdraw, maxWithdrawPercent);
+        super.test_reverts_whenWithdrawExceedsMaxAllowed(accountBalance, withdraw, MaxWithdrawDenominator);
     }
 
     function _validateInvokingCall() internal override {

From ab2361b3a0692ebfaa82ea024daf7c4c17b0797b Mon Sep 17 00:00:00 2001
From: Wilson Cusack <wilson.cusack@coinbase.com>
Date: Thu, 4 Apr 2024 14:54:18 -0400
Subject: [PATCH 5/5] natspec

---
 src/MagicSpend.sol | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/MagicSpend.sol b/src/MagicSpend.sol
index 6073702..8d33a1c 100644
--- a/src/MagicSpend.sol
+++ b/src/MagicSpend.sol
@@ -30,7 +30,7 @@ contract MagicSpend is Ownable, IPaymaster {
         uint48 expiry;
     }
 
-    /// @notice address.balance divided by maxWithdrawDenominator expresses the max WithdrawRequest.amount
+    /// @notice address(this).balance divided by maxWithdrawDenominator expresses the max WithdrawRequest.amount
     /// allowed for a native asset withdraw.
     ///
     /// @dev Helps prevent withdraws in the same transaction leading to reverts and hurting paymaster reputation.