Skip to content

M04 - minBptFunction robustness #1795

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Aug 30, 2023
Merged

M04 - minBptFunction robustness #1795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f7b0394
change bptExpected to ignore Oracle prices and assume assets are alwa…
sparrowDom Aug 28, 2023
b12dfed
fix all the tests
sparrowDom Aug 28, 2023
feae3f7
add test for pool manipulation
sparrowDom Aug 29, 2023
79d2466
prettier & lint
sparrowDom Aug 29, 2023
9d08767
minor change
sparrowDom Aug 29, 2023
cddf1ae
add withdrawal test
sparrowDom Aug 29, 2023
cf06cf5
update documentation
sparrowDom Aug 29, 2023
ff99af8
pick string error length that is smaller than 32 characters
sparrowDom Aug 30, 2023
ec927b6
Merge branch 'sparrowDom/balancer-sfrxETH-stETH-rETH' into sparrowDom…
sparrowDom Aug 30, 2023
635a2dd
prettier
sparrowDom Aug 30, 2023
bf24d1e
correct comment
sparrowDom Aug 30, 2023
2b76644
better comments
sparrowDom Aug 30, 2023
96437f1
prettier
sparrowDom Aug 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion brownie/abi/balancer_strat.json
View file
Open in desktop

Large diffs are not rendered by default.

109 changes: 58 additions & 51 deletions contracts/contracts/strategies/balancer/BalancerMetaPoolStrategy.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,36 +28,30 @@ contract BalancerMetaPoolStrategy is BaseAuraStrategy {
{}

/**
* @notice Deposits an `_amount` of vault collateral assets
* from the this strategy contract to the Balancer pool.
* @param _strategyAsset Address of the Vault collateral asset
* @param _strategyAmount The amount of Vault collateral assets to deposit
* @notice There are no plans to configure BalancerMetaPool as a default
* asset strategy. For that reason there is no need to support this
* functionality.
*/
function deposit(address _strategyAsset, uint256 _strategyAmount)
function deposit(address, uint256)
external
override
onlyVault
nonReentrant
{
address[] memory strategyAssets = new address[](1);
uint256[] memory strategyAmounts = new uint256[](1);
strategyAssets[0] = _strategyAsset;
strategyAmounts[0] = _strategyAmount;

_deposit(strategyAssets, strategyAmounts);
revert("Not supported");
}

/**
* @notice Deposits specified vault collateral assets
* from the this strategy contract to the Balancer pool.
* @param _strategyAssets Address of the Vault collateral assets
* @param _strategyAmounts The amount of each asset to deposit
* @notice There are no plans to configure BalancerMetaPool as a default
* asset strategy. For that reason there is no need to support this
* functionality.
*/
function deposit(
address[] memory _strategyAssets,
uint256[] memory _strategyAmounts
) external onlyVault nonReentrant {
_deposit(_strategyAssets, _strategyAmounts);
function deposit(address[] memory, uint256[] memory)
external
onlyVault
nonReentrant
{
revert("Not supported");
}

/**
Expand All @@ -72,7 +66,9 @@ contract BalancerMetaPoolStrategy is BaseAuraStrategy {
for (uint256 i = 0; i < assetsLength; ++i) {
strategyAssets[i] = assetsMapped[i];
// Get the asset balance in this strategy contract
strategyAmounts[i] = IERC20(strategyAssets[i]).balanceOf(address(this));
strategyAmounts[i] = IERC20(strategyAssets[i]).balanceOf(
address(this)
);
}
_deposit(strategyAssets, strategyAmounts);
}
Expand Down Expand Up @@ -133,8 +129,13 @@ contract BalancerMetaPoolStrategy is BaseAuraStrategy {
}
}

uint256 minBPT = getBPTExpected(_strategyAssets, _strategyAmounts);
uint256 minBPTwSlippage = minBPT.mulTruncate(1e18 - maxDepositSlippage);
uint256 minBPT = getBPTExpected(
strategyAssetsToPoolAssets,
strategyAssetAmountsToPoolAssetAmounts
);
uint256 minBPTwDeviation = minBPT.mulTruncate(
1e18 - maxDepositDeviation
);

/* EXACT_TOKENS_IN_FOR_BPT_OUT:
* User sends precise quantities of tokens, and receives an
Expand All @@ -146,7 +147,7 @@ contract BalancerMetaPoolStrategy is BaseAuraStrategy {
bytes memory userData = abi.encode(
IBalancerVault.WeightedPoolJoinKind.EXACT_TOKENS_IN_FOR_BPT_OUT,
amountsIn,
minBPTwSlippage
minBPTwDeviation
);

IBalancerVault.JoinPoolRequest memory request = IBalancerVault
Expand Down Expand Up @@ -214,30 +215,13 @@ contract BalancerMetaPoolStrategy is BaseAuraStrategy {
);

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

@DanielVF DanielVF Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

require(assetToPToken[_strategyAssets[i]] != address(0), "Unsupported asset");
require(
assetToPToken[_strategyAssets[i]] != address(0),
"Unsupported asset"
);
}

// STEP 1 - Calculate the max about of Balancer Pool Tokens (BPT) to withdraw

// Estimate the required amount of Balancer Pool Tokens (BPT) for the assets
uint256 maxBPTtoWithdraw = getBPTExpected(
_strategyAssets,
_strategyAmounts
);
// Increase BPTs by the max allowed slippage
// Any excess BPTs will be left in this strategy contract
maxBPTtoWithdraw = maxBPTtoWithdraw.mulTruncate(
1e18 + maxWithdrawalSlippage
);

// STEP 2 - Withdraw the Balancer Pool Tokens (BPT) from Aura to this strategy contract

// Withdraw BPT from Aura allowing for BPTs left in this strategy contract from previous withdrawals
_lpWithdraw(
maxBPTtoWithdraw - IERC20(platformAddress).balanceOf(address(this))
);

// STEP 3 - Calculate the Balancer pool assets and amounts from the vault collateral assets
// STEP 1 - Calculate the Balancer pool assets and amounts from the vault collateral assets

// Get all the supported balancer pool assets
(IERC20[] memory tokens, , ) = balancerVault.getPoolTokens(
Expand Down Expand Up @@ -287,6 +271,29 @@ contract BalancerMetaPoolStrategy is BaseAuraStrategy {
}
}

// STEP 2 - Calculate the max about of Balancer Pool Tokens (BPT) to withdraw

// Estimate the required amount of Balancer Pool Tokens (BPT) for the assets
uint256 maxBPTtoWithdraw = getBPTExpected(
poolAssets,
/* all non 0 values are overshot by 2 WEI and with the expected mainnet
* ~1% withdrawal deviation, the 2 WEI aren't important
*/
poolAssetsAmountsOut
);
// Increase BPTs by the max allowed deviation
// Any excess BPTs will be left in this strategy contract
maxBPTtoWithdraw = maxBPTtoWithdraw.mulTruncate(
1e18 + maxWithdrawalDeviation
);

// STEP 3 - Withdraw the Balancer Pool Tokens (BPT) from Aura to this strategy contract

// Withdraw BPT from Aura allowing for BPTs left in this strategy contract from previous withdrawals
_lpWithdraw(
maxBPTtoWithdraw - IERC20(platformAddress).balanceOf(address(this))
);

// STEP 4 - Withdraw the balancer pool assets from the pool

/* Custom asset exit: BPT_IN_FOR_EXACT_TOKENS_OUT:
Expand Down Expand Up @@ -327,10 +334,10 @@ contract BalancerMetaPoolStrategy is BaseAuraStrategy {
);

// 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
* boosted yield. We re-deploy those back in.
/* When concluding how much of BPT we need to withdraw from Aura we overshoot by
* roughly around 1% (initial mainnet setting of maxWithdrawalDeviation). After exiting
* the pool strategy could have left over BPT tokens that are not earning boosted yield.
* We re-deploy those back in.
*/
_lpDepositAll();

Expand Down Expand Up @@ -395,7 +402,7 @@ contract BalancerMetaPoolStrategy is BaseAuraStrategy {
poolAssets[i] = address(tokens[i]);
minAmountsOut[i] = balances[i]
.mulTruncate(strategyShare)
.mulTruncate(1e18 - maxWithdrawalSlippage);
.mulTruncate(1e18 - maxWithdrawalDeviation);
}

// STEP 3 - Withdraw the Balancer pool assets from the pool
Expand Down
132 changes: 66 additions & 66 deletions contracts/contracts/strategies/balancer/BaseBalancerStrategy.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@ abstract contract BaseBalancerStrategy is InitializableAbstractStrategy {
/// @notice Balancer pool identifier
bytes32 public immutable balancerPoolId;

// Max withdrawal slippage denominated in 1e18 (1e18 == 100%)
uint256 public maxWithdrawalSlippage;
// Max deposit slippage denominated in 1e18 (1e18 == 100%)
uint256 public maxDepositSlippage;
// Max withdrawal deviation denominated in 1e18 (1e18 == 100%)
uint256 public maxWithdrawalDeviation;
// Max deposit deviation denominated in 1e18 (1e18 == 100%)
uint256 public maxDepositDeviation;

int256[48] private __reserved;

Expand All @@ -48,13 +48,13 @@ abstract contract BaseBalancerStrategy is InitializableAbstractStrategy {
bytes32 balancerPoolId; // Balancer pool identifier
}

event MaxWithdrawalSlippageUpdated(
uint256 _prevMaxSlippagePercentage,
uint256 _newMaxSlippagePercentage
event MaxWithdrawalDeviationUpdated(
uint256 _prevMaxDeviationPercentage,
uint256 _newMaxDeviationPercentage
);
event MaxDepositSlippageUpdated(
uint256 _prevMaxSlippagePercentage,
uint256 _newMaxSlippagePercentage
event MaxDepositDeviationUpdated(
uint256 _prevMaxDeviationPercentage,
uint256 _newMaxDeviationPercentage
);

/**
Expand Down Expand Up @@ -99,11 +99,11 @@ abstract contract BaseBalancerStrategy is InitializableAbstractStrategy {
address[] calldata _assets,
address[] calldata _pTokens
) external override onlyGovernor initializer {
maxWithdrawalSlippage = 1e15;
maxDepositSlippage = 1e15;
maxWithdrawalDeviation = 1e16;
maxDepositDeviation = 1e16;

emit MaxWithdrawalSlippageUpdated(0, maxWithdrawalSlippage);
emit MaxDepositSlippageUpdated(0, maxDepositSlippage);
emit MaxWithdrawalDeviationUpdated(0, maxWithdrawalDeviation);
emit MaxDepositDeviationUpdated(0, maxDepositDeviation);

IERC20[] memory poolAssets = getPoolAssets();
require(
Expand Down Expand Up @@ -225,28 +225,37 @@ abstract contract BaseBalancerStrategy is InitializableAbstractStrategy {

/* solhint-disable max-line-length */
/**
* @notice BPT price is calculated by dividing the pool (sometimes wrapped) market price by the
* rateProviderRate of that asset. To get BPT expected we need to multiply that by underlying
* asset amount divided by BPT token rate. BPT token rate is similar to Curve's virtual_price
* and expresses how much has the price of BPT appreciated in relation to the underlying assets.
* @notice BPT price is calculated by taking the rate from the rateProvider of the asset in
* question. If one does not exist it defaults to 1e18. To get the final BPT expected that
* is multiplied by the underlying asset amount divided by BPT token rate. BPT token rate is
* similar to Curve's virtual_price and expresses how much has the price of BPT appreciated
* (e.g. due to swap fees) in relation to the underlying assets
*
* @dev
* bptPrice = pool_asset_oracle_price / pool_asset_rate
* Using the above approach makes the strategy vulnerable to a possible MEV attack using
* flash loan to manipulate the pool before a deposit/withdrawal since the function ignores
* market values of the assets being priced in BPT.
*
* At the time of writing there is no safe on-chain approach to pricing BPT in a way that it
* would make it invulnerable to MEV pool manipulation. See recent Balancer exploit:
* https://www.notion.so/originprotocol/Balancer-OETH-strategy-9becdea132704e588782a919d7d471eb?pvs=4#1cf07de12fc64f1888072321e0644348
*
* Since we only have oracle prices for the unwrapped version of the assets the equation
* turns into:
* 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.
*
* bptPrice = from_pool_token(asset_amount).amount * oracle_price / pool_asset_rate
* @param _asset Address of the Balancer pool asset
* @param _amount Amount of the Balancer pool asset
* @return bptExpected of BPT expected in exchange for the asset
*
* bptExpected = bptPrice(in relation to specified asset) * asset_amount / BPT_token_rate
* @dev
* bptAssetPrice = 1e18 (asset peg) * pool_asset_rate
*
* and since from_pool_token(asset_amount).amount and pool_asset_rate cancel each-other out
* this makes the final equation:
* bptExpected = bptAssetPrice * asset_amount / BPT_token_rate
*
* bptExpected = oracle_price * asset_amount / BPT_token_rate
* bptExpected = 1e18 (asset peg) * pool_asset_rate * asset_amount / BPT_token_rate
* bptExpected = asset_amount * pool_asset_rate / BPT_token_rate
*
* more explanation here:
* https://www.notion.so/originprotocol/Support-Balancer-OETH-strategy-9becdea132704e588782a919d7d471eb?pvs=4#382834f9815e46a7937f3acca0f637c5
* further information available here:
* https://www.notion.so/originprotocol/Balancer-OETH-strategy-9becdea132704e588782a919d7d471eb?pvs=4#ce01495ae70346d8971f5dced809fb83
*/
/* solhint-enable max-line-length */
function getBPTExpected(address _asset, uint256 _amount)
Expand All @@ -255,13 +264,9 @@ abstract contract BaseBalancerStrategy is InitializableAbstractStrategy {
virtual
returns (uint256 bptExpected)
{
address priceProvider = IVault(vaultAddress).priceProvider();
uint256 strategyAssetMarketPrice = IOracle(priceProvider).price(_asset);
uint256 bptRate = IRateProvider(platformAddress).getRate();

bptExpected = _amount
.mulTruncate(strategyAssetMarketPrice)
.divPrecisely(bptRate);
uint256 poolAssetRate = getRateProviderRate(_asset);
bptExpected = _amount.mulTruncate(poolAssetRate).divPrecisely(bptRate);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

function getBPTExpected(address[] memory _assets, uint256[] memory _amounts)
Expand All @@ -270,17 +275,12 @@ abstract contract BaseBalancerStrategy is InitializableAbstractStrategy {
virtual
returns (uint256 bptExpected)
{
// Get the oracle from the OETH Vault
address priceProvider = IVault(vaultAddress).priceProvider();
require(_assets.length == _amounts.length, "Assets & amounts mismatch");

for (uint256 i = 0; i < _assets.length; ++i) {
uint256 strategyAssetMarketPrice = IOracle(priceProvider).price(
_assets[i]
);
uint256 poolAssetRate = getRateProviderRate(_assets[i]);
// convert asset amount to ETH amount
bptExpected =
bptExpected +
_amounts[i].mulTruncate(strategyAssetMarketPrice);
bptExpected += _amounts[i].mulTruncate(poolAssetRate);
}

uint256 bptRate = IRateProvider(platformAddress).getRate();
Expand Down Expand Up @@ -431,50 +431,50 @@ abstract contract BaseBalancerStrategy is InitializableAbstractStrategy {
}

/**
* @notice Sets max withdrawal slippage that is considered when removing
* @notice Sets max withdrawal deviation that is considered when removing
* liquidity from Balancer pools.
* @param _maxWithdrawalSlippage Max withdrawal slippage denominated in
* @param _maxWithdrawalDeviation Max withdrawal deviation denominated in
* wad (number with 18 decimals): 1e18 == 100%, 1e16 == 1%
*
* IMPORTANT Minimum maxWithdrawalSlippage should actually be 0.1% (1e15)
* for production usage. Contract allows as low value as 0% for confirming
* correct behavior in test suite.
* IMPORTANT Minimum maxWithdrawalDeviation will be 1% (1e16) for production
* usage. Vault value checker in combination with checkBalance will
* catch any unexpected manipulation.
*/
function setMaxWithdrawalSlippage(uint256 _maxWithdrawalSlippage)
function setMaxWithdrawalDeviation(uint256 _maxWithdrawalDeviation)
external
onlyVaultOrGovernorOrStrategist
{
require(
_maxWithdrawalSlippage <= 1e18,
"Max withdrawal slippage needs to be between 0% - 100%"
_maxWithdrawalDeviation <= 1e18,
"Withdrawal dev. out of bounds"
);
emit MaxWithdrawalSlippageUpdated(
maxWithdrawalSlippage,
_maxWithdrawalSlippage
emit MaxWithdrawalDeviationUpdated(
maxWithdrawalDeviation,
_maxWithdrawalDeviation
);
maxWithdrawalSlippage = _maxWithdrawalSlippage;
maxWithdrawalDeviation = _maxWithdrawalDeviation;
}

/**
* @notice Sets max deposit slippage that is considered when adding
* @notice Sets max deposit deviation that is considered when adding
* liquidity to Balancer pools.
* @param _maxDepositSlippage Max deposit slippage denominated in
* @param _maxDepositDeviation Max deposit deviation denominated in
* wad (number with 18 decimals): 1e18 == 100%, 1e16 == 1%
*
* IMPORTANT Minimum maxDepositSlippage should actually be 0.1% (1e15)
* for production usage. Contract allows as low value as 0% for confirming
* correct behavior in test suite.
* IMPORTANT Minimum maxDepositDeviation will default to 1% (1e16)
* for production usage. Vault value checker in combination with
* checkBalance will catch any unexpected manipulation.
*/
function setMaxDepositSlippage(uint256 _maxDepositSlippage)
function setMaxDepositDeviation(uint256 _maxDepositDeviation)
external
onlyVaultOrGovernorOrStrategist
{
require(
_maxDepositSlippage <= 1e18,
"Max deposit slippage needs to be between 0% - 100%"
require(_maxDepositDeviation <= 1e18, "Deposit dev. out of bounds");
emit MaxDepositDeviationUpdated(
maxDepositDeviation,
_maxDepositDeviation
);
emit MaxDepositSlippageUpdated(maxDepositSlippage, _maxDepositSlippage);
maxDepositSlippage = _maxDepositSlippage;
maxDepositDeviation = _maxDepositDeviation;
}

function _approveBase() internal virtual {
Expand Down
Loading