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

Weighted Pool: ongoing reentrancy protection #2330

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

Conversation

EndymionJkb
Copy link
Collaborator

@EndymionJkb EndymionJkb commented Mar 8, 2023

Description

WeightedPool (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 in the deployment fork test)
  • 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:31
@EndymionJkb
Copy link
Collaborator Author

Thanks, @jubeira !

@jubeira
Copy link
Contributor

jubeira commented Mar 8, 2023

Fixed compile issue with message UnimplementedFeatureError: Encoding type "struct IVault.UserBalanceOp memory[] memory" not yet implemented. (missing pragma experimental ABIEncoderV2) in 596aea8.

@nventuro
Copy link
Contributor

nventuro commented Mar 8, 2023

This was the patch change: #2205

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 #2205, except that one also included changes to updateProtocolFeePercentageCache and this one doesn't (because that was added in #2296).

@nventuro
Copy link
Contributor

nventuro commented Mar 8, 2023

Incidentally #2296 did not add tests for the protocol fee cache update, but it should have.

*/
function disableRecoveryMode() external override authenticate {
// Some derived contracts respond to disabling recovery mode with state changes (e.g., related to protocol fees,
// or otherwise ensuring that enabling and disabling recovery mode has no ill effects on LPs). When called
// outside of recovery mode, these state changes might lead to unexpected behavior.
_ensureInRecoveryMode();

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.

This should be accompanied by a test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Side comment (maybe for a follow-up PR): we are doing this here and in ProtocolFeeCache (which is derived from RecoveryMode) without a modifier.

Wouldn't it be more consistent to define the modifier here, and use it in both contracts? That way the code would be the same both in pools and in these auxiliary contracts.

Copy link
Collaborator Author

@EndymionJkb EndymionJkb Mar 9, 2023

Choose a reason for hiding this comment

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

Yes; I thought of that, but it was spread over separate PRs, so I did them individually. Will do a follow-on to address that. (Perhaps I have too little faith in git merge :)

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.

Changes look good; the functions that needed to be patched / documented are covered.

Regarding tests: same comment as in stable patch applies: #2331 (review)

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