Balancer Composable stable pool strategy

[sparrowDom]

Adds support for Balancer's Composable stable pools.

Connected PR: #1849

[shahthepro]

This and _exactBptInTokensOutIndex can be constants (can define it in BalancerMetaPoolStrategy and override it here)

[sparrowDom]

This is a solidity thing where you can not override constants. Interestingly (written in the same link) you can override a public function in a child contract using an overridden constant if the signatures and return types match.

[shahthepro]

poolAssetIndex[poolAsset] is being read twice

[sparrowDom]

This is a great catch. Thanks for the gas optimization tip. done here: 8fb75ec

[shahthepro]

nit: Perhaps rename it to bptToWithdraw? Also, there's no bptToWithdraw > 0 check anywhere. It's gonna fail IVault.withdrawAllFromStrategies (although we have a faultTolerantWithdrawAllFromStrategies method, would be nice to surface this error)

[sparrowDom]

yes uncapitalized is a better way to name it thanks, changed.

Regarding withdrawAll, we can withdraw on an empty strategy and it doesn't revert. Tested here That said there is no need to spend gas when there is nothing to withdraw. Test, renames and gas optimisation done here: 6dbb9cd

[shahthepro]

There're some failing tests, other than that it looks good ot me

Deployment Considerations

Internal State

• Two new constants are added at the top of the existing contract (BaseBalancerStrategy), however since they're constant they don't affect the storage layout
• BaseBalancerStrategy.__reserved is updated correctly after adding three new state variables

Potential Attacks/Edge Cases

• I suspected that doing multiple withdrawAll and depositAll back to back would cause withdrawAll to fail at some point (due to the usage of FRX_ETH_REDEEM_CORRECTION) however it turned out to be not true due a few other checks we have that ensures that there would no dust left. Besides we always resort to faultTolerantWithdrawAllFromStrategies when/if withdrawAll fails

Logic

• Are there bugs in the logic? No
• Correct usage of global & local variables. -> they might differentiate only by an underscore that can be overlooked (e.g. address vs _address).
• Do public/external functions having asset parameter verify the asset is supported (where applicable)

Tests

• Some fork and unit tests are failing on CI
• Each publicly callable method has a test
• Each logical branch has a test
• Each require() has a test
• Edge conditions are tested
• If tests interact with AMM make sure enough edge cases (pool tilts) are tested. Ideally with fuzzing.

Flavor

Could this code be simpler?

Could this code be less vulnerable to other code behaving weirdly?

Overflow

• Never use "+" or "-", always use safe math or have contract compile in solidity version > 0.8
• Check that all for loops use uint256

Proxy

• Make sure proxy implementation contracts don't initialize variable state on variable declaration and do it rather in initialize function.

Black magic

• Does not contain selfdestruct
• Does not use delegatecall outside of proxying
• (If an implementation contract were to call delegatecall under attacker control, it could call selfdestruct the implementation contract, leading to calls through the proxy silently succeeding, even though they were failing.)
• Address.isContract should be treated as if could return anything at any time, because that's reality.

Dependencies

• Review any new contract dependencies thoroughly (e.g. OpenZeppelin imports) when new dependencies are added or version of dependencies changes.
• If OpenZeppelin ACL roles are use review & enumerate all of them.
• Check OpenZeppelin security vulnerabilities and see if any apply to current PR considering the version of OpenZeppelin contract used.

Deploy

• Check that any deployer permissions are removed after deploy

Authentication

• Never use tx.origin
• Check that every external/public function should actually be external
• Check that every external/public function has the correct authentication

Cryptographic code

• Contracts that roll their own crypto are terrifying
• Note that a failed signature check will result in a 0x00 result. Make sure that the result throws if it returns this.
• Beware of signed data being used in a replay attack to other contracts.

Gas problems

• Contracts with for loops must have either:
  • A way to remove items
  • Can be upgraded to get unstuck
  • Size can only controlled by admins
• Contracts with for loops must not allow end users to add unlimited items to a loop that is used by others or admins.

External calls

• Contract addresses passed in are validated
• No unsafe external calls
• Reentrancy guards on all state changing functions
  • Still doesn't protect against external contracts changing the state of the world if they are called.
• No malicious behaviors
• Should not fail from stack depth problems (low level calls must require success)
• No slippage attacks (we need to validate expected tokens received)
• Oracles, one of:
  • No oracles
  • Oracles can't be bent
  • If oracle can be bent, it won't hurt us.
• Don't call balanceOf for external contracts to determine what they will do, when they instead use internal accounting?

Ethereum

• Contract does not send or receive Ethereum.
• Contract has no payable methods.

[sparrowDom]

@shahthepro the fork tests related to this PR have been fixed

[pandadefi]

I see you are using ++i to save a little bit of gas, this addition could be performed using uncheck.
It's also possible to add uncheck to the following ++i on line 116 and 131

[sparrowDom]

That is a great point thanks. I have just read up on this how; Solidity (after 0.8.0) doesn't need to perform overflow checks in situations where no overflow is guaranteed. And looping through arrays definitely is one of those situations.

The unchecked does provide some gas savings with a little minor impact on code readability. In this case I would rather not make the change, since first loop is done only on initialisation. And the other 2 are done on join/exit pools. We are running those operations relatively rarely - and are paying for the gas ourselves.

If these optimisations would be part of checkBalance function ( that gets called in Vault's allocate/rebase that are called in most of mints & redeems) then I'd gladly trade the readability impact for the gas savings.

And thanks I will keep the unchecked in mind for future optimization opportunities.

[pandadefi]

I am still learning about this strategy and the parent strategies. I would like to understand why the BPT token amount needs to be removed. Could you give me some explanations?

[sparrowDom]

Hey sure, so the Composable Stable Pools have this particularity that they have the BPT token positioned next to other pool assets. This unlocks the ability of more easily nesting / batch swapping with other Balancer pools.

When joining/exiting a Balancer pool (add liquidity / remove liquidity in Curve's terms) encoded userData needs to be passed along the request that encodes expected tokens received after the join/exit. And that function expects the amounts not to have the BPT token amount present in the array (even though it is one of the assets in the Composable Stable Pool).

Additionally when we are calculating the amount of BPT expected to be received from withdrawing given assets the BPT token needs to be omitted from the calculation. Since we are swapping the BPT token in exchange for the pool assets.

Meta stable pools (predecessors of Composable Stable Pools) didn't have the BPT token present as one of the pool's assets and thus the exclusion of BPT from the join/exit requests wasn't necessary.

Does that make sense?