M04 - minBptFunction robustness #1795

sparrowDom · 2023-08-29T21:12:16Z

Things in this PR:

minBptExpected function ignores oracle prices and considers only rate of rateProviders. Meaning that strategy is assuming that assets are pegged
fork tests added to check that with pool manipulation checkBalance almost isn't affected
fork tests added to check that with pool manipulation depositing is safe when using VaultValueChecker
deposit functions (that would be used by Vault if this were ever an asset default strategy) disabled
maxWithdrawal/DepositSlippage renamed to maxWithdrawal/DepositDeviation

From audit report:
The getBPTExpected functions are used to calculate the minimum amount of BPTs to
receive when depositing liquidity, or the maximum amount of BPTs to provide when
withdrawing liquidity. They calculate the underlying ETH value of the assets to deposit/
withdraw, and divide it by the price of a BPT in terms of the pool's underlying base asset.
There are two issues with this:
Based on which oracles are being used, the strategy's asset price might be stale, which
will result in an over/underestimation of the correct number of BPTs.
The calculation assumes normal market conditions, namely that the pool's underlying
base asset is trading at 1-1 parity with ETH. stETH or frxETH depegging from ETH
would make this untrue, resulting in an overestimation of the amount of BPTs.
Overestimating the BPT amount can result in reverts during deposits while underestimating
them can result in reverts during withdrawals. Consider accurately calculating the exact
number of BPTs required for these operations using Balancer's StableMath.

…ys pegged. (Vault value checker shall be used to mitigate issues with said assumption)

contracts/contracts/strategies/balancer/BaseBalancerStrategy.sol

…/M04-minBptFunction

contracts/contracts/strategies/balancer/BaseBalancerStrategy.sol

contracts/test/strategies/balancerPoolReentrancy.fork-test.js

DanielVF · 2023-08-30T17:35:20Z

contracts/contracts/strategies/balancer/BaseBalancerStrategy.sol

-            .mulTruncate(strategyAssetMarketPrice)
-            .divPrecisely(bptRate);
+        uint256 poolAssetRate = getRateProviderRate(_asset);
+        bptExpected = _amount.mulTruncate(poolAssetRate).divPrecisely(bptRate);


We don't have to change anything here but,

This:

bptExpected = _amount.mulTruncate(poolAssetRate).divPrecisely(bptRate);

is doing this:

bptExpected = ((_amount * poolAssetRate / 1e18) * 1e18 / bptRate);

Which is the same as

bptExpected = _amount * poolAssetRate / bptRate

This stable math library we are using is also using the safe math library under the hood, which adds unneeded overhead to the contracts.

Again, no change needed, just something to think about for the future.

I haven't though of that, good point. The mulTruncate + divPrecisely could cancel the 1e18 multiplication & division out.

We could do that change, though adding a comment would probably make sense, since the though process can easily be forgotten.

DanielVF · 2023-08-30T18:31:09Z

I think the oracle portion of this comment may be out of date:

  // STEP 5 - Re-deposit any left over BPT tokens back into Aura
  /* When concluding how much of BPT we need to withdraw from Aura we rely on Oracle prices
   * and those can be stale (most ETH based have 24 hour heartbeat & 2% price change trigger)
   * After exiting the pool strategy could have left over BPT tokens that are not earning

DanielVF · 2023-08-30T19:04:33Z

contracts/contracts/strategies/balancer/BalancerMetaPoolStrategy.sol

@@ -214,30 +215,13 @@ contract BalancerMetaPoolStrategy is BaseAuraStrategy {
        );

        for (uint256 i = 0; i < _strategyAssets.length; ++i) {


Fun to think about what would happen if we passed in the same asset multiple times in the _strategyAssets array and _strategyAmounts array.

I don't think there would be an issue. The last one would set the amount used for the withdraw, but after the the the withdraw the strategy would try to transfer all of the amounts out, which should fail, since only the last amount was withdrawn from the strategy.

Yes that is a great thought. Quickly glancing through the code I would expect this inner loop: https://github.com/OriginProtocol/origin-dollar/blob/sparrowDom/balancer-sfrxETH-stETH-rETH/contracts/contracts/strategies/balancer/BalancerMetaPoolStrategy.sol#L265-L293 to only consider the last entry in the strategyAssets & _strategyAmount of the duplicated assets to be considered. And it would withdraw too little from the balancer pool and pay too much in BPT tokens.

And as you've said the ERC20 transfer out as the final step of the function should fail.

DanielVF · 2023-08-30T19:13:48Z

contracts/contracts/strategies/balancer/BaseBalancerStrategy.sol

     *
-     * Since we only have oracle prices for the unwrapped version of the assets the equation
-     * turns into:
+     * To mitigate MEV possibilities VaultValueChecker in conjunction with checkBalance will catch


This sentence could be "To mitigate MEV possibilities during deposits and withdraws, the VaultValueChecker will use checkBalance before and after the move to ensure the expected changes took place."

thanks much better!

sparrowDom · 2023-08-30T19:57:19Z

thanks @DanielVF nice catch regarding the oracle comment. Have corrected that one as well.

notion-workspace · 2023-09-11T15:22:39Z

Fork tests for balancer pool tilt manipulation

* initail commit * intermediary commit * commit research files * balancer booster abi * intermittent commit * add base balancer contract that implements checkBalance functionality * add some additional initial integration * intermittent commit * add deployment file * add fork test fixture * intermittent commit * prettier * add basic withdrawal / deposit functionality * correct the BPT calculation * prettier + lint * simplify the BPT price calculation * add read-only re-entrancy protection * add some missing tests and adjust existing. Deparate deposit and withdrawal slippage * fix check balance implementation * Balancer review changes (#1726) * Generated contract docs * Refactor Balancer storage variables * Small Balancer changes * Natspec updates Added missing licensing getPoolAssets gas optimized * Updated generated Balancer strategy contract diagrams * fix contract linter * Removed restrictions on tests * Small gas improvements Fixed Slither * Change BalancerError version * Updated constant names Addresses to use checksum format * JS lint tasks * Updated Balancer and Aura pool id constants * Removed getRateProviderRate as it wasn't being used Refactored to remove poolAssetsMapped storage variable * Updated OETH Contracts diagrams Generated new Balancer contract diagrams * Fix failing test * Fix merge conflict * Restored getRateProviderRate * Natspec updates Added toPoolAsset override * Removed unused getRateProviderRate * Natspec updates Gas optimization of InitializableAbstractStrategy * Abstract strategy gas improvements (#1719) * Refactor base strategy to use immutables * Fixed strategy deployments in 001_core and fixtures * Generated new strategy diagrams * Deploy rETH instead of the stETH Balancer MetaStable Pool * removed unused Aura config * Balancer fork tests * Added check that BPT amount equals Aura LP amount Added rETH conversion to ETH value * Updated balancer strat fork tests * Updated Balancer fork tests * Added optional deposit with multiple assets to the strategy * Single asset deposit to use multi asset deposit * Added optional checkBalance to Balancer strategy * Added checkBalance() to BaseBalancerStrategy * Fix slither Fix curve HH task * Added multi-asset withdraw to balancer strategy * Fix multi-asset withdraw * Updated Balancer and Vault diagrams * Fix js linter * Fixed checkBalance of rETH asset in Balancer strategy * Only wrap assets if amount > 0 Added depositAll fork test for Balancer strat * Removed Vault changes for multi-asset strategy support * Updated generated docs * Add tests for wstETH/WETH Balancer pool (#1725) * Split deployment and fix fixtures * Deposit tests for wstETH/WETH pool * Add withdraw test * prettier * remove .only in fork tests --------- Co-authored-by: Shahul Hameed <[email protected]> * [ DFD-1 ] Balancer's checkBalance (#1730) * add alternative implementation of Balancer's checkBalance * correct the checkBalance implementation * Balancer fork tests (#1727) * Added large withdraw tests for Balancer strategy * fix test log * Balancer withdraw to handle close to BPT limit * Small change to Balancer withdraw fork test * add some comments * change implementation --------- Co-authored-by: Domen Grabec <[email protected]> * Add read-only reentrancy test (#1731) * Added large withdraw tests for Balancer strategy * fix test log * Balancer withdraw to handle close to BPT limit * Small change to Balancer withdraw fork test * add some comments * Add test for read-only reentrancy * add reentrancy protection to checkBalance functions * add documentation * remove the only --------- Co-authored-by: Nicholas Addison <[email protected]> Co-authored-by: Domen Grabec <[email protected]> * Balancer fixes (#1734) * prettier * adjust how checkBalance is calculated * Balancer withdrawal fix (#1739) * fix balancer withdrawals * lint * prettier * use only 1 safeApprove when applicable * some renames and more correct application of approves * renames, additional requires, move initializer to a better location, slither * bug fix * Generated latest Balancer strategy diagrams * re-deploy BPT tokens sitting in the strategy * fix re-entrancy test * fixture fix * bug fix * prettier * L02 improve naming (#1783) * improve naming * one more rename * buf fix * do a check that supported assets are being withdrawn (#1784) * set uint256 max instead of magic number (#1782) * remove unused files (#1785) * fix renaming bug * correct safe approve all tokens and adjust the documentation (#1776) * prettier * M04 - minBptFunction robustness (#1795) * change bptExpected to ignore Oracle prices and assume assets are always pegged. (Vault value checker shall be used to mitigate issues with said assumption) * fix all the tests * add test for pool manipulation * prettier & lint * minor change * add withdrawal test * update documentation * pick string error length that is smaller than 32 characters * prettier * correct comment * better comments * prettier * M02 withdrawal fuzzing tests (#1801) * add more withdrawal tests * add more withdrawal cases * N02 gas inefficiencies (#1786) * gas optimisation * fix bad merge and prettier * remove todo comments (#1796) * use a more appropriate array initialisation length (#1800) * more consistant function naming (#1797) * fix typo (#1798) * simplify the way we withdrawAll. no need to pass along min amonts (#1777) * M03 - missing or misleading documentation (#1781) * improve documentation * add comma * M01 More comprehensive test suite (#1780) * add tests for approving tokens and harvesting rewards * prettier and lint * fix bad merge + prettier & lint * fix fork tests remove .only * remove only * lint * fix unit tests * add more tests to see how checkBalance behaves * remove console log * improve checkBalance test by testing that checkBalance amount doesn't decrease after the attack comaring to the middle of the attack. * confirm that yield gained by 3rd party tilting the pool can be extracted by withdrawing the funds * rename internal functions by prepending them with underscore * Generated latest Balancer strategy diagrams (#1805) * bug fix * bug fix * Minor Balancer changes from final review (#1819) * Removed unused imports * Generated updated contract diagram --------- Co-authored-by: Nick Addison <[email protected]> Co-authored-by: Shahul Hameed <[email protected]>

* initail commit * intermediary commit * commit research files * balancer booster abi * intermittent commit * add base balancer contract that implements checkBalance functionality * add some additional initial integration * intermittent commit * add deployment file * add fork test fixture * intermittent commit * prettier * add basic withdrawal / deposit functionality * correct the BPT calculation * prettier + lint * simplify the BPT price calculation * add read-only re-entrancy protection * add some missing tests and adjust existing. Deparate deposit and withdrawal slippage * fix check balance implementation * Balancer review changes (#1726) * Generated contract docs * Refactor Balancer storage variables * Small Balancer changes * Natspec updates Added missing licensing getPoolAssets gas optimized * Updated generated Balancer strategy contract diagrams * fix contract linter * Removed restrictions on tests * Small gas improvements Fixed Slither * Change BalancerError version * Updated constant names Addresses to use checksum format * JS lint tasks * Updated Balancer and Aura pool id constants * Removed getRateProviderRate as it wasn't being used Refactored to remove poolAssetsMapped storage variable * Updated OETH Contracts diagrams Generated new Balancer contract diagrams * Fix failing test * Fix merge conflict * Restored getRateProviderRate * Natspec updates Added toPoolAsset override * Removed unused getRateProviderRate * Natspec updates Gas optimization of InitializableAbstractStrategy * Abstract strategy gas improvements (#1719) * Refactor base strategy to use immutables * Fixed strategy deployments in 001_core and fixtures * Generated new strategy diagrams * Deploy rETH instead of the stETH Balancer MetaStable Pool * removed unused Aura config * Balancer fork tests * Added check that BPT amount equals Aura LP amount Added rETH conversion to ETH value * Updated balancer strat fork tests * Updated Balancer fork tests * Added optional deposit with multiple assets to the strategy * Single asset deposit to use multi asset deposit * Added optional checkBalance to Balancer strategy * Added checkBalance() to BaseBalancerStrategy * Fix slither Fix curve HH task * Added multi-asset withdraw to balancer strategy * Fix multi-asset withdraw * Updated Balancer and Vault diagrams * Fix js linter * Fixed checkBalance of rETH asset in Balancer strategy * Only wrap assets if amount > 0 Added depositAll fork test for Balancer strat * Removed Vault changes for multi-asset strategy support * Updated generated docs * Add tests for wstETH/WETH Balancer pool (#1725) * Split deployment and fix fixtures * Deposit tests for wstETH/WETH pool * Add withdraw test * prettier * remove .only in fork tests --------- Co-authored-by: Shahul Hameed <[email protected]> * [ DFD-1 ] Balancer's checkBalance (#1730) * add alternative implementation of Balancer's checkBalance * correct the checkBalance implementation * Balancer fork tests (#1727) * Added large withdraw tests for Balancer strategy * fix test log * Balancer withdraw to handle close to BPT limit * Small change to Balancer withdraw fork test * add some comments * change implementation --------- Co-authored-by: Domen Grabec <[email protected]> * Add read-only reentrancy test (#1731) * Added large withdraw tests for Balancer strategy * fix test log * Balancer withdraw to handle close to BPT limit * Small change to Balancer withdraw fork test * add some comments * Add test for read-only reentrancy * add reentrancy protection to checkBalance functions * add documentation * remove the only --------- Co-authored-by: Nicholas Addison <[email protected]> Co-authored-by: Domen Grabec <[email protected]> * Balancer fixes (#1734) * prettier * adjust how checkBalance is calculated * Balancer withdrawal fix (#1739) * fix balancer withdrawals * lint * prettier * use only 1 safeApprove when applicable * some renames and more correct application of approves * renames, additional requires, move initializer to a better location, slither * bug fix * Generated latest Balancer strategy diagrams * re-deploy BPT tokens sitting in the strategy * fix re-entrancy test * fixture fix * bug fix * prettier * L02 improve naming (#1783) * improve naming * one more rename * buf fix * do a check that supported assets are being withdrawn (#1784) * set uint256 max instead of magic number (#1782) * remove unused files (#1785) * fix renaming bug * correct safe approve all tokens and adjust the documentation (#1776) * prettier * M04 - minBptFunction robustness (#1795) * change bptExpected to ignore Oracle prices and assume assets are always pegged. (Vault value checker shall be used to mitigate issues with said assumption) * fix all the tests * add test for pool manipulation * prettier & lint * minor change * add withdrawal test * update documentation * pick string error length that is smaller than 32 characters * prettier * correct comment * better comments * prettier * M02 withdrawal fuzzing tests (#1801) * add more withdrawal tests * add more withdrawal cases * N02 gas inefficiencies (#1786) * gas optimisation * fix bad merge and prettier * remove todo comments (#1796) * use a more appropriate array initialisation length (#1800) * more consistant function naming (#1797) * fix typo (#1798) * simplify the way we withdrawAll. no need to pass along min amonts (#1777) * M03 - missing or misleading documentation (#1781) * improve documentation * add comma * M01 More comprehensive test suite (#1780) * add tests for approving tokens and harvesting rewards * prettier and lint * fix bad merge + prettier & lint * fix fork tests remove .only * remove only * lint * fix unit tests * add more tests to see how checkBalance behaves * remove console log * improve checkBalance test by testing that checkBalance amount doesn't decrease after the attack comaring to the middle of the attack. * confirm that yield gained by 3rd party tilting the pool can be extracted by withdrawing the funds * rename internal functions by prepending them with underscore * Generated latest Balancer strategy diagrams (#1805) * bug fix * bug fix * Minor Balancer changes from final review (#1819) * Removed unused imports * Generated updated contract diagram * Deploy Balancer Meta stable pool RETH strategy * update deploy description * fix typo * add proposal Id to deploy script * prettier --------- Co-authored-by: Nick Addison <[email protected]> Co-authored-by: Shahul Hameed <[email protected]>

* initail commit * intermediary commit * commit research files * balancer booster abi * intermittent commit * add base balancer contract that implements checkBalance functionality * add some additional initial integration * intermittent commit * add deployment file * add fork test fixture * intermittent commit * prettier * add basic withdrawal / deposit functionality * correct the BPT calculation * prettier + lint * simplify the BPT price calculation * add read-only re-entrancy protection * add some missing tests and adjust existing. Deparate deposit and withdrawal slippage * fix check balance implementation * Balancer review changes (#1726) * Generated contract docs * Refactor Balancer storage variables * Small Balancer changes * Natspec updates Added missing licensing getPoolAssets gas optimized * Updated generated Balancer strategy contract diagrams * fix contract linter * Removed restrictions on tests * Small gas improvements Fixed Slither * Change BalancerError version * Updated constant names Addresses to use checksum format * JS lint tasks * Updated Balancer and Aura pool id constants * Removed getRateProviderRate as it wasn't being used Refactored to remove poolAssetsMapped storage variable * Updated OETH Contracts diagrams Generated new Balancer contract diagrams * Fix failing test * Fix merge conflict * Restored getRateProviderRate * Natspec updates Added toPoolAsset override * Removed unused getRateProviderRate * Natspec updates Gas optimization of InitializableAbstractStrategy * Abstract strategy gas improvements (#1719) * Refactor base strategy to use immutables * Fixed strategy deployments in 001_core and fixtures * Generated new strategy diagrams * Deploy rETH instead of the stETH Balancer MetaStable Pool * removed unused Aura config * Balancer fork tests * Added check that BPT amount equals Aura LP amount Added rETH conversion to ETH value * Updated balancer strat fork tests * Updated Balancer fork tests * Added optional deposit with multiple assets to the strategy * Single asset deposit to use multi asset deposit * Added optional checkBalance to Balancer strategy * Added checkBalance() to BaseBalancerStrategy * Fix slither Fix curve HH task * Added multi-asset withdraw to balancer strategy * Fix multi-asset withdraw * Updated Balancer and Vault diagrams * Fix js linter * Fixed checkBalance of rETH asset in Balancer strategy * Only wrap assets if amount > 0 Added depositAll fork test for Balancer strat * Removed Vault changes for multi-asset strategy support * Updated generated docs * Add tests for wstETH/WETH Balancer pool (#1725) * Split deployment and fix fixtures * Deposit tests for wstETH/WETH pool * Add withdraw test * prettier * remove .only in fork tests --------- Co-authored-by: Shahul Hameed <[email protected]> * [ DFD-1 ] Balancer's checkBalance (#1730) * add alternative implementation of Balancer's checkBalance * correct the checkBalance implementation * Balancer fork tests (#1727) * Added large withdraw tests for Balancer strategy * fix test log * Balancer withdraw to handle close to BPT limit * Small change to Balancer withdraw fork test * add some comments * change implementation --------- Co-authored-by: Domen Grabec <[email protected]> * Add read-only reentrancy test (#1731) * Added large withdraw tests for Balancer strategy * fix test log * Balancer withdraw to handle close to BPT limit * Small change to Balancer withdraw fork test * add some comments * Add test for read-only reentrancy * add reentrancy protection to checkBalance functions * add documentation * remove the only --------- Co-authored-by: Nicholas Addison <[email protected]> Co-authored-by: Domen Grabec <[email protected]> * Balancer fixes (#1734) * prettier * adjust how checkBalance is calculated * Balancer withdrawal fix (#1739) * fix balancer withdrawals * lint * prettier * use only 1 safeApprove when applicable * some renames and more correct application of approves * renames, additional requires, move initializer to a better location, slither * bug fix * Generated latest Balancer strategy diagrams * re-deploy BPT tokens sitting in the strategy * fix re-entrancy test * fixture fix * bug fix * prettier * L02 improve naming (#1783) * improve naming * one more rename * buf fix * do a check that supported assets are being withdrawn (#1784) * set uint256 max instead of magic number (#1782) * remove unused files (#1785) * fix renaming bug * correct safe approve all tokens and adjust the documentation (#1776) * prettier * M04 - minBptFunction robustness (#1795) * change bptExpected to ignore Oracle prices and assume assets are always pegged. (Vault value checker shall be used to mitigate issues with said assumption) * fix all the tests * add test for pool manipulation * prettier & lint * minor change * add withdrawal test * update documentation * pick string error length that is smaller than 32 characters * prettier * correct comment * better comments * prettier * M02 withdrawal fuzzing tests (#1801) * add more withdrawal tests * add more withdrawal cases * N02 gas inefficiencies (#1786) * gas optimisation * fix bad merge and prettier * remove todo comments (#1796) * use a more appropriate array initialisation length (#1800) * more consistant function naming (#1797) * fix typo (#1798) * simplify the way we withdrawAll. no need to pass along min amonts (#1777) * M03 - missing or misleading documentation (#1781) * improve documentation * add comma * M01 More comprehensive test suite (#1780) * add tests for approving tokens and harvesting rewards * prettier and lint * fix bad merge + prettier & lint * fix fork tests remove .only * remove only * lint * fix unit tests * add more tests to see how checkBalance behaves * remove console log * improve checkBalance test by testing that checkBalance amount doesn't decrease after the attack comaring to the middle of the attack. * confirm that yield gained by 3rd party tilting the pool can be extracted by withdrawing the funds * rename internal functions by prepending them with underscore * Generated latest Balancer strategy diagrams (#1805) * bug fix * bug fix * Minor Balancer changes from final review (#1819) * Removed unused imports * Generated updated contract diagram * Deploy Balancer Meta stable pool RETH strategy * update deploy description * fix typo * add proposal Id to deploy script * prettier * create a deploy file * file rename * improve wording * fix name redundancy --------- Co-authored-by: Nick Addison <[email protected]> Co-authored-by: Shahul Hameed <[email protected]>

* initail commit * intermediary commit * commit research files * balancer booster abi * intermittent commit * add base balancer contract that implements checkBalance functionality * add some additional initial integration * intermittent commit * add deployment file * add fork test fixture * intermittent commit * prettier * add basic withdrawal / deposit functionality * correct the BPT calculation * prettier + lint * simplify the BPT price calculation * add read-only re-entrancy protection * add some missing tests and adjust existing. Deparate deposit and withdrawal slippage * fix check balance implementation * Balancer review changes (#1726) * Generated contract docs * Refactor Balancer storage variables * Small Balancer changes * Natspec updates Added missing licensing getPoolAssets gas optimized * Updated generated Balancer strategy contract diagrams * fix contract linter * Removed restrictions on tests * Small gas improvements Fixed Slither * Change BalancerError version * Updated constant names Addresses to use checksum format * JS lint tasks * Updated Balancer and Aura pool id constants * Removed getRateProviderRate as it wasn't being used Refactored to remove poolAssetsMapped storage variable * Updated OETH Contracts diagrams Generated new Balancer contract diagrams * Fix failing test * Fix merge conflict * Restored getRateProviderRate * Natspec updates Added toPoolAsset override * Removed unused getRateProviderRate * Natspec updates Gas optimization of InitializableAbstractStrategy * Abstract strategy gas improvements (#1719) * Refactor base strategy to use immutables * Fixed strategy deployments in 001_core and fixtures * Generated new strategy diagrams * Deploy rETH instead of the stETH Balancer MetaStable Pool * removed unused Aura config * Balancer fork tests * Added check that BPT amount equals Aura LP amount Added rETH conversion to ETH value * Updated balancer strat fork tests * Updated Balancer fork tests * Added optional deposit with multiple assets to the strategy * Single asset deposit to use multi asset deposit * Added optional checkBalance to Balancer strategy * Added checkBalance() to BaseBalancerStrategy * Fix slither Fix curve HH task * Added multi-asset withdraw to balancer strategy * Fix multi-asset withdraw * Updated Balancer and Vault diagrams * Fix js linter * Fixed checkBalance of rETH asset in Balancer strategy * Only wrap assets if amount > 0 Added depositAll fork test for Balancer strat * Removed Vault changes for multi-asset strategy support * Updated generated docs * Add tests for wstETH/WETH Balancer pool (#1725) * Split deployment and fix fixtures * Deposit tests for wstETH/WETH pool * Add withdraw test * prettier * remove .only in fork tests --------- Co-authored-by: Shahul Hameed <[email protected]> * [ DFD-1 ] Balancer's checkBalance (#1730) * add alternative implementation of Balancer's checkBalance * correct the checkBalance implementation * Balancer fork tests (#1727) * Added large withdraw tests for Balancer strategy * fix test log * Balancer withdraw to handle close to BPT limit * Small change to Balancer withdraw fork test * add some comments * change implementation --------- Co-authored-by: Domen Grabec <[email protected]> * Add read-only reentrancy test (#1731) * Added large withdraw tests for Balancer strategy * fix test log * Balancer withdraw to handle close to BPT limit * Small change to Balancer withdraw fork test * add some comments * Add test for read-only reentrancy * add reentrancy protection to checkBalance functions * add documentation * remove the only --------- Co-authored-by: Nicholas Addison <[email protected]> Co-authored-by: Domen Grabec <[email protected]> * Balancer fixes (#1734) * prettier * adjust how checkBalance is calculated * Balancer withdrawal fix (#1739) * fix balancer withdrawals * lint * prettier * use only 1 safeApprove when applicable * some renames and more correct application of approves * renames, additional requires, move initializer to a better location, slither * bug fix * Generated latest Balancer strategy diagrams * re-deploy BPT tokens sitting in the strategy * fix re-entrancy test * fixture fix * bug fix * prettier * L02 improve naming (#1783) * improve naming * one more rename * buf fix * do a check that supported assets are being withdrawn (#1784) * set uint256 max instead of magic number (#1782) * remove unused files (#1785) * fix renaming bug * correct safe approve all tokens and adjust the documentation (#1776) * prettier * M04 - minBptFunction robustness (#1795) * change bptExpected to ignore Oracle prices and assume assets are always pegged. (Vault value checker shall be used to mitigate issues with said assumption) * fix all the tests * add test for pool manipulation * prettier & lint * minor change * add withdrawal test * update documentation * pick string error length that is smaller than 32 characters * prettier * correct comment * better comments * prettier * M02 withdrawal fuzzing tests (#1801) * add more withdrawal tests * add more withdrawal cases * N02 gas inefficiencies (#1786) * gas optimisation * fix bad merge and prettier * remove todo comments (#1796) * use a more appropriate array initialisation length (#1800) * more consistant function naming (#1797) * fix typo (#1798) * simplify the way we withdrawAll. no need to pass along min amonts (#1777) * M03 - missing or misleading documentation (#1781) * improve documentation * add comma * M01 More comprehensive test suite (#1780) * add tests for approving tokens and harvesting rewards * prettier and lint * fix bad merge + prettier & lint * fix fork tests remove .only * remove only * lint * fix unit tests * add more tests to see how checkBalance behaves * remove console log * improve checkBalance test by testing that checkBalance amount doesn't decrease after the attack comaring to the middle of the attack. * confirm that yield gained by 3rd party tilting the pool can be extracted by withdrawing the funds * rename internal functions by prepending them with underscore * Generated latest Balancer strategy diagrams (#1805) * bug fix * bug fix * Minor Balancer changes from final review (#1819) * Removed unused imports * Generated updated contract diagram * Deploy Balancer Meta stable pool RETH strategy * update deploy description * fix typo * add proposal Id to deploy script * prettier * create a deploy file * file rename * improve wording * fix name redundancy * re-deploy Balancer rETH/WETH strategy * add proposal id * improve deploy proposal wording * update proposal id * Prettify --------- Co-authored-by: Nick Addison <[email protected]> Co-authored-by: Shahul Hameed <[email protected]>

sparrowDom added 5 commits August 29, 2023 01:24

change bptExpected to ignore Oracle prices and assume assets are alwa…

f7b0394

…ys pegged. (Vault value checker shall be used to mitigate issues with said assumption)

fix all the tests

b12dfed

add test for pool manipulation

feae3f7

prettier & lint

79d2466

minor change

9d08767

sparrowDom requested review from DanielVF, naddison36 and shahthepro as code owners August 29, 2023 21:12

rafaelugolini temporarily deployed to preview-oeth-sparrowdom-em4opj August 29, 2023 21:12 Inactive

rafaelugolini temporarily deployed to preview-ousd-sparrowdom-4hsksf August 29, 2023 21:12 Inactive

add withdrawal test

cddf1ae

sparrowDom temporarily deployed to preview-oeth-sparrowdom-em4opj August 29, 2023 22:39 Inactive

sparrowDom temporarily deployed to preview-ousd-sparrowdom-4hsksf August 29, 2023 22:39 Inactive

update documentation

cf06cf5

sparrowDom temporarily deployed to preview-oeth-sparrowdom-em4opj August 29, 2023 22:51 Inactive

sparrowDom temporarily deployed to preview-ousd-sparrowdom-4hsksf August 29, 2023 22:51 Inactive

sparrowDom mentioned this pull request Aug 29, 2023

M03 - missing or misleading documentation #1781

Merged

3 tasks

naddison36 approved these changes Aug 30, 2023

View reviewed changes

contracts/contracts/strategies/balancer/BaseBalancerStrategy.sol Outdated Show resolved Hide resolved

pick string error length that is smaller than 32 characters

ff99af8

sparrowDom temporarily deployed to preview-ousd-sparrowdom-4hsksf August 30, 2023 10:22 Inactive

sparrowDom temporarily deployed to preview-oeth-sparrowdom-em4opj August 30, 2023 10:22 Inactive

sparrowDom added 2 commits August 30, 2023 12:23

Merge branch 'sparrowDom/balancer-sfrxETH-stETH-rETH' into sparrowDom…

ec927b6

…/M04-minBptFunction

prettier

635a2dd

sparrowDom temporarily deployed to preview-oeth-sparrowdom-em4opj August 30, 2023 10:24 Inactive

sparrowDom temporarily deployed to preview-ousd-sparrowdom-4hsksf August 30, 2023 10:24 Inactive

DanielVF reviewed Aug 30, 2023

View reviewed changes

contracts/contracts/strategies/balancer/BaseBalancerStrategy.sol Outdated Show resolved Hide resolved

DanielVF reviewed Aug 30, 2023

View reviewed changes

contracts/test/strategies/balancerPoolReentrancy.fork-test.js Show resolved Hide resolved

DanielVF reviewed Aug 30, 2023

View reviewed changes

correct comment

bf24d1e

sparrowDom temporarily deployed to preview-ousd-sparrowdom-4hsksf August 30, 2023 19:04 Inactive

sparrowDom temporarily deployed to preview-oeth-sparrowdom-em4opj August 30, 2023 19:04 Inactive

DanielVF reviewed Aug 30, 2023

View reviewed changes

DanielVF approved these changes Aug 30, 2023

View reviewed changes

sparrowDom mentioned this pull request Aug 30, 2023

M01 More comprehensive test suite #1780

Merged

4 tasks

sparrowDom added 2 commits August 30, 2023 21:57

better comments

2b76644

prettier

96437f1

sparrowDom merged commit 6ad405c into sparrowDom/balancer-sfrxETH-stETH-rETH Aug 30, 2023

sparrowDom deleted the sparrowDom/M04-minBptFunction branch August 30, 2023 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

M04 - minBptFunction robustness #1795

M04 - minBptFunction robustness #1795

Uh oh!

sparrowDom commented Aug 29, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanielVF Aug 30, 2023

Uh oh!

sparrowDom Aug 30, 2023

Uh oh!

DanielVF commented Aug 30, 2023

Uh oh!

DanielVF Aug 30, 2023 •

edited

Loading

Uh oh!

sparrowDom Aug 30, 2023

Uh oh!

DanielVF Aug 30, 2023

Uh oh!

sparrowDom Aug 30, 2023

Uh oh!

sparrowDom commented Aug 30, 2023

Uh oh!

notion-workspace bot commented Sep 11, 2023

Uh oh!

Uh oh!

		@@ -214,30 +215,13 @@ contract BalancerMetaPoolStrategy is BaseAuraStrategy {
		);

		for (uint256 i = 0; i < _strategyAssets.length; ++i) {

M04 - minBptFunction robustness #1795

M04 - minBptFunction robustness #1795

Uh oh!

Conversation

sparrowDom commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanielVF Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

sparrowDom Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

DanielVF commented Aug 30, 2023

Uh oh!

DanielVF Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sparrowDom Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

DanielVF Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

sparrowDom Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

sparrowDom commented Aug 30, 2023

Uh oh!

notion-workspace bot commented Sep 11, 2023

Uh oh!

Uh oh!

sparrowDom commented Aug 29, 2023 •

edited

Loading

DanielVF Aug 30, 2023 •

edited

Loading