Guard against concurrent withdraws causing unexpected reverts #17
Conversation
|
Partially remediates code-423n4/2024-03-coinbase-findings#110 |
There was a problem hiding this comment.
As-is, I think this all looks fine.
The approach implemented here is not written in a way that makes it imminently compatible with other assets. I know we only plan on supporting ETH to start, but is it worth future-proofing this while we're at it?
Perhaps a mapping maxWithdrawDenominatorByAsset and maybe logging the asset in the error/events.
src/MagicSpend.sol
Outdated
| /// @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; |
There was a problem hiding this comment.
I think the naming of the var ...Denominator and the natspec description as percentage clash a bit. Perhaps re-word the natspec to The maximum fraction of address.balance... to explicitly describe the way this is used as a rate.
There was a problem hiding this comment.
Agree naming/comment wording is a bit misleading.
There was a problem hiding this comment.
@stevieraykatz @xenoliss updated naming
There was a problem hiding this comment.
I would rather have reworded the comment I guess because now maxWithdrawPercent is used in a denominator and it seems a bit weird.
This is only a concern for our paymaster reputation. Given we can only be a paymaster when withdraw asset is ETH, and we'd need a code change to change that, I feel good about keeping it more limited. |
cb-heimdall
dismissed
xenoliss’s stale review
Approved review 1980013308 from xenoliss is now dismissed due to new commit. Re-request for approval.
src/MagicSpend.sol
Outdated
| /// @dev Percent expressed in whole units, e.g. 20 means no single withdraw | ||
| /// can exceed 20% of address.balance |
There was a problem hiding this comment.
The comment does not match with the actual code logic:
uint256 maxAllowed = address(this).balance / maxWithdrawPercent;
cb-heimdall
dismissed
stevieraykatz’s stale review
Approved review 1980863332 from stevieraykatz is now dismissed due to new commit. Re-request for approval.
Supersedes #16, which doesn't really fix the problem because reverts in bundle simulation hurt our reputation no matter where they come from (validation, execution, etc.)
Alternative approach here is to add a
maxWithdrawDenominatorwhich can be set to configure the max number of concurrent, non-reverting ETH withdraws in a single transaction.