L-01 Duplicate Strategy Assets Can Be Passed to _withdraw #1913
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sparrowDom
requested review from
DanielVF,
naddison36 and
shahthepro
as code owners
rafaelugolini
temporarily deployed
to
preview-ousd-sparrowdom-rdsj33
DanielVF
reviewed
Nov 7, 2023
| @@ -157,6 +158,14 @@ contract BalancerMetaPoolStrategy is BaseAuraStrategy { | |||
| strategyAmount | |||
| ); | |||
|
|
|||
| /* This check is triggered when the _deposit is unexpectedly | |||
There was a problem hiding this comment.
Probably don't need the word "unexpectedly" here.
DanielVF
reviewed
Nov 7, 2023
| @@ -300,6 +309,14 @@ contract BalancerMetaPoolStrategy is BaseAuraStrategy { | |||
| if (poolAssetAmount > 0) { | |||
| strategyAssetsToPoolAssetsAmounts[i] = poolAssetAmount; | |||
|
|
|||
| /* This check is triggered when the _withdrawal is unexpectedly | |||
There was a problem hiding this comment.
Same, don't need "unexpectedly"
DanielVF
approved these changes
Nov 7, 2023
…/L-01-duplicate-assets
sparrowDom
requested a deployment
to
preview-ousd-sparrowdom-rdsj33
sparrowDom
merged commit e0005ec
into
sparrowDom/balancer-composable-st-pool
1 check passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a check so withdrawal with duplicate assets can not be called. Currently, the deposit with duplicate assets can not be called (contract API doesn't support it) but a future proof check is still added (with a test) to validate duplicate asset deposit calls can also not be made.
Audit comments:
The array arguments passed to
_withdraware not validated to ensure that they containunique assets. If, in a future extension, it becomes possible to pass duplicates (either due to
user or strategist input), the following impacts may occur:
Consider enforcing that poolAssetsAmountsOut[..] is 0 before setting its value.