fix: keep track of pending withdrawals #16
Conversation
| revert InsufficientBalance(withdrawAmount, address(this).balance); | ||
| uint256 newPendingWithdrawals = _pendingWithdrawals + withdrawAmount; | ||
|
|
||
| if ((newPendingWithdrawals - 1) > address(this).balance) { |
There was a problem hiding this comment.
not sure it's worth the gas / visual complexity of subtracting the 1 wei :)
There was a problem hiding this comment.
Do you recommend removing it entirely?
The point of this PR was to track the amounts to avoid our paymaster reputation being decreased in case where the bundler would include userops that revert at execution due to insufficient balance. If I remove this I wonder if someone could not use this off-by-one issue intentionally.
There was a problem hiding this comment.
hmm I don't see a way to exploit this. In this case we are OVER estimating pending withdraws? So the failure case there is a withdraw that gets rejected in validation but could have succeeded?
There was a problem hiding this comment.
Not implemented in this PR, will reconsider it for later.
test/ValidatePaymasterUserOp.t.sol
Outdated
| amount = withdrawAmount; | ||
|
|
||
| vm.deal(address(magic), initialBalance); | ||
| vm.store(address(magic), bytes32(uint256(0)), bytes32(pendingWithdrawals + 1)); |
There was a problem hiding this comment.
might be better to test multiple calls to validate so that we ensure we are setting / updating correctly
There was a problem hiding this comment.
The best approach might be to have a test that simulates the bundler (validation + execution) and keep those unit tests focused on ~1 use case.
There was a problem hiding this comment.
We have integration tests here if you want to write new one https://github.com/coinbase/magic-spend/blob/main/test/Integration.t.sol
I think we could have here test_reverts_whenBalanceTooLow1Withdraw test_reverts_whenBalanceTooLow2Withdraws
There was a problem hiding this comment.
I replaced the current test implementation to perform two successive validations that exhaust (and exceed) the available balance.
|
Closing as a simpler and effective approach is preferred (see #17) |
This PR fixes code-423n4/2024-03-coinbase-findings#110.
Implementation changes:
_pendingWithdrawalsstate variable whose initial value is set to 1 (for gas)validatePaymasterUserOp():available balance = balance - _pendingWithdrawals)._pendingWithdrawalspostOp()reset_pendingWithdrawalsto 1 if neededUnexpectedPostOpRevertedModeinpostOp()instead of asserting.Tests changes:
Minor refactoring, the biggest changes being in
test_revertsInsufficientAvailableBalance()to add fuzzing.