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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guard against concurrent withdraws causing unexpected reverts #17

Merged
merged 5 commits into from
Apr 4, 2024

Conversation

wilsoncusack
Copy link
Contributor

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 maxWithdrawDenominator which can be set to configure the max number of concurrent, non-reverting ETH withdraws in a single transaction.

@wilsoncusack
Copy link
Contributor Author

Partially remediates code-423n4/2024-03-coinbase-findings#110

Copy link
Contributor

@stevieraykatz stevieraykatz left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines 33 to 35
/// @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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree naming/comment wording is a bit misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stevieraykatz @xenoliss updated naming

Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather have reworded the comment I guess because now maxWithdrawPercent is used in a denominator and it seems a bit weird.

src/MagicSpend.sol Show resolved Hide resolved
@wilsoncusack
Copy link
Contributor Author

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.

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.

xenoliss
xenoliss previously approved these changes Apr 4, 2024
@cb-heimdall cb-heimdall dismissed xenoliss’s stale review April 4, 2024 17:10

Approved review 1980013308 from xenoliss is now dismissed due to new commit. Re-request for approval.

Comment on lines 37 to 38
/// @dev Percent expressed in whole units, e.g. 20 means no single withdraw
/// can exceed 20% of address.balance
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment does not match with the actual code logic:

uint256 maxAllowed = address(this).balance / maxWithdrawPercent;

stevieraykatz
stevieraykatz previously approved these changes Apr 4, 2024
@cb-heimdall cb-heimdall dismissed stevieraykatz’s stale review April 4, 2024 18:54

Approved review 1980863332 from stevieraykatz is now dismissed due to new commit. Re-request for approval.

@wilsoncusack wilsoncusack merged commit 0041c9d into main Apr 4, 2024
3 of 5 checks passed
@wilsoncusack wilsoncusack deleted the wilson/reputation-fix branch April 4, 2024 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants