Audit Fix: Fix coins.add usage during deposit #342
Merged
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
added 2 commits
November 1, 2023 15:34
every sdk.Coin.Add happening in DepositCore results in multiple traversals over all coins in sharesIssued , and also involves allocation of additional data structures, thus resulting in quadratic scaling of the DepositCore runtime (because it invokes sdk.Coins Add() for each issued share).
Much better gas efficiency since we don't need to perform duplicate lookups for each share issuance Also add additional check for duplicate deposits. This should exist regardless but also required for share math to work if we aren't issuing shares within the deposit loop
pr0n00gler
approved these changes
Nov 3, 2023
sotnikov-s
reviewed
Nov 8, 2023
sotnikov-s
approved these changes
Nov 9, 2023
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.
Every sdk.Coin.Add happening in DepositCore results in multiple traversals over all coins
in sharesIssued , and also involves allocation of additional data structures, thus resulting in quadratic
scaling of the DepositCore runtime (because it invokes sdk.Coins Add() for each issued share).
As a fix, we instead aggregate the coins as a slice and then perform aggregation and validation only once.
We also move coin sending logic to the end of the deposit function for further efficiency.