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

Stable Pool: ongoing reentrancy protection #2331

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

EndymionJkb
Copy link
Collaborator

@EndymionJkb EndymionJkb commented Mar 8, 2023

Description

ComposableStablePool (version 3) was patched for reentrancy; this applies the same (generalized) protections so that any future deployments are safe.

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Dependency changes
  • Code refactor / cleanup
  • Documentation or wording changes
  • Other

Checklist:

  • The diff is legible and has no extraneous changes
  • [N/A] Complex code has been commented, including external interfaces
  • [N/A] Tests are included for all code paths (would be fork tests in the deployment)
  • The base branch is either master, or there's a description of how to merge

Issue Resolution

@EndymionJkb EndymionJkb marked this pull request as draft March 8, 2023 11:12
@EndymionJkb EndymionJkb marked this pull request as ready for review March 8, 2023 20:26
@nventuro
Copy link
Contributor

nventuro commented Mar 8, 2023

This was the patch change: #2206

Copy link
Contributor

@jubeira jubeira left a comment

Choose a reason for hiding this comment

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

First pass done.

Besides the inline comment,

  • don't we also need to protect updateTokenRateCache in ComposableStablePoolRates?
  • what about disableRecoveryMode? It already has access to the vault, but I don't see the protection applied in this branch either. EDIT: it's added in Weighted Pool: ongoing reentrancy protection #2330

@@ -141,6 +142,8 @@ abstract contract ComposableStablePoolRates is IComposableStablePoolRates, Compo

/// @inheritdoc IComposableStablePoolRates
function setTokenRateCacheDuration(IERC20 token, uint256 duration) external override authenticate {
VaultReentrancyLib.ensureNotInVaultContext(_getVault());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not define a modifier and use it here, like we did for the patch?

Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a test

Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

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

This seems to apply the same behavior as #2206, except that one also included changes to updateProtocolFeePercentageCache and disableRecoveryMode and this one doesnt (because those were added in #2296 and #2330 respectively).

@EndymionJkb
Copy link
Collaborator Author

Yes; didn't want to potentially conflict.

Copy link
Contributor

@jubeira jubeira left a comment

Choose a reason for hiding this comment

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

Patch looks good; all the functions that should be protected and documented are covered with this.

I've just left one super minor comment regarding a comment.

About the tests: we could include them in the fork tests as we did with the patches instead of doing unit tests. I don't have a strong opinion regarding what's the best way forward. WDYT @nventuro ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants