From 1c0f9888a11e497ab495b059640b3bac55296f59 Mon Sep 17 00:00:00 2001 From: Domen Grabec Date: Mon, 13 Nov 2023 18:49:34 +0100 Subject: [PATCH 1/2] add comment and change enum types to uint8 --- .../BalancerComposablePoolStrategy.sol | 8 +++---- .../balancer/BalancerMetaPoolStrategy.sol | 22 ++++++------------- .../balancerMetaStablePool.fork-test.js | 21 ++++++++++++++++++ 3 files changed, 32 insertions(+), 19 deletions(-) diff --git a/contracts/contracts/strategies/balancer/BalancerComposablePoolStrategy.sol b/contracts/contracts/strategies/balancer/BalancerComposablePoolStrategy.sol index 81024267f0..aabf3141c1 100644 --- a/contracts/contracts/strategies/balancer/BalancerComposablePoolStrategy.sol +++ b/contracts/contracts/strategies/balancer/BalancerComposablePoolStrategy.sol @@ -47,10 +47,10 @@ contract BalancerComposablePoolStrategy is BalancerMetaPoolStrategy { internal pure override - returns (uint256) + returns (uint8) { return - uint256( + uint8( IBalancerVault .ComposablePoolExitKind .BPT_IN_FOR_EXACT_TOKENS_OUT @@ -65,10 +65,10 @@ contract BalancerComposablePoolStrategy is BalancerMetaPoolStrategy { internal pure override - returns (uint256) + returns (uint8) { return - uint256( + uint8( IBalancerVault .ComposablePoolExitKind .EXACT_BPT_IN_FOR_ALL_TOKENS_OUT diff --git a/contracts/contracts/strategies/balancer/BalancerMetaPoolStrategy.sol b/contracts/contracts/strategies/balancer/BalancerMetaPoolStrategy.sol index 106e03fc0b..97806ca4ae 100644 --- a/contracts/contracts/strategies/balancer/BalancerMetaPoolStrategy.sol +++ b/contracts/contracts/strategies/balancer/BalancerMetaPoolStrategy.sol @@ -18,7 +18,7 @@ contract BalancerMetaPoolStrategy is BaseAuraStrategy { using StableMath for uint256; /// @dev Special ExitKind for all Balancer pools, used in Recovery Mode. - uint256 constant RECOVERY_MODE_EXIT_KIND = 255; + uint8 constant RECOVERY_MODE_EXIT_KIND = 255; int256[50] private ___reserved; @@ -37,14 +37,9 @@ contract BalancerMetaPoolStrategy is BaseAuraStrategy { * request exactly specifies the amount of underlying assets * to be returned. */ - function _btpInExactTokensOutIndex() - internal - pure - virtual - returns (uint256) - { + function _btpInExactTokensOutIndex() internal pure virtual returns (uint8) { return - uint256( + uint8( IBalancerVault .MetaStablePoolExitKind .BPT_IN_FOR_EXACT_TOKENS_OUT @@ -55,14 +50,9 @@ contract BalancerMetaPoolStrategy is BaseAuraStrategy { * @dev enum Value that represents exit encoding where BPT tokens are supplied for * proportional exit is required when calling a withdrawAll. */ - function _exactBptInTokensOutIndex() - internal - pure - virtual - returns (uint256) - { + function _exactBptInTokensOutIndex() internal pure virtual returns (uint8) { return - uint256( + uint8( IBalancerVault .MetaStablePoolExitKind .EXACT_BPT_IN_FOR_TOKENS_OUT @@ -169,6 +159,8 @@ contract BalancerMetaPoolStrategy is BaseAuraStrategy { /* This check is triggered when the _deposit is called with * a duplicate asset in the _strategyAssets array + * + * */ require( amountsIn[assetIndex] == 0, diff --git a/contracts/test/strategies/balancerMetaStablePool.fork-test.js b/contracts/test/strategies/balancerMetaStablePool.fork-test.js index c1a98f7fa1..f4f851548d 100644 --- a/contracts/test/strategies/balancerMetaStablePool.fork-test.js +++ b/contracts/test/strategies/balancerMetaStablePool.fork-test.js @@ -550,6 +550,27 @@ describe("ForkTest: Balancer MetaStablePool rETH/WETH Strategy", function () { ).to.be.revertedWith("No duplicate withdrawal assets"); }); + /* Ideally this would also revert, but that would make the withdrawal function more gas expensive + * and doesn't seem like a good trade-off. + */ + it(`Should succeed when duplicating an asset and first amount being 0`, async function () { + const { balancerREthStrategy, oethVault, weth } = fixture; + + const oethVaultSigner = await impersonateAndFund(oethVault.address); + const zeroAmount = await units("0", weth); + const wethWithdrawAmount = await units("1", weth); + + // prettier-ignore + await expect( + balancerREthStrategy + .connect(oethVaultSigner)["withdraw(address,address[],uint256[])"]( + oethVault.address, + [weth.address, weth.address], + [zeroAmount, wethWithdrawAmount] + ) + ).to.not.be.reverted; + }); + it("Should be able to withdraw all of pool liquidity", async function () { const { oethVault, weth, reth, balancerREthStrategy } = fixture; From ff7c11ab95cba33c623fb05d0191fd27bf2483f1 Mon Sep 17 00:00:00 2001 From: Domen Grabec Date: Mon, 13 Nov 2023 19:00:01 +0100 Subject: [PATCH 2/2] add better comment --- .../contracts/strategies/balancer/BalancerMetaPoolStrategy.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/contracts/strategies/balancer/BalancerMetaPoolStrategy.sol b/contracts/contracts/strategies/balancer/BalancerMetaPoolStrategy.sol index 97806ca4ae..0e4f7e0e54 100644 --- a/contracts/contracts/strategies/balancer/BalancerMetaPoolStrategy.sol +++ b/contracts/contracts/strategies/balancer/BalancerMetaPoolStrategy.sol @@ -160,7 +160,8 @@ contract BalancerMetaPoolStrategy is BaseAuraStrategy { /* This check is triggered when the _deposit is called with * a duplicate asset in the _strategyAssets array * - * + * A duplicate asset supplied with 0 amount or an amount close to + * 0 that wraps to a 0 amount will still pass this check. */ require( amountsIn[assetIndex] == 0,