BalancerV2 Feed

[sicknick99]

Adds two contracts to create markets using BalancerV2 pools:

• OverlayV1BalancerV2Factory
• OverlayV1BalancerV2Feed

[mikeyrf]

Left some initial review notes above. Going to come back to the more substantial pieces OverlayV1BalancerV2Factory and OverlayV1BalancerV2Feed + associated tests tomorrow to review. :)

[mikeyrf]

Why GPL vs MIT?

[sicknick99]

Can change to MIT. Which do you prefer? My fav is CC 1.0

[mikeyrf]

MIT please!

[mikeyrf]

Passing in BalancerV2Tokens.Info struct as a deploy feed function input is vulnerable to manipulation: I can deploy a feed with the vault address as my own malicious contract that implements the vault interface, but returns the wrong data to feed.

deployFeed() therefore needs a check that the struct info

library BalancerV2Tokens {
    struct Info {
        address vault;
        bytes32 ovlWethPoolId;
        bytes32 marketPoolId;
    }
}

data is valid. See here how I do this with Uniswap, but you want to use a Balancer "trusted" factory contract or the address of the vault itself as the check. Then get the pool tokens from the vault. The vault address can be stored as an immutable in this feed factory contract, and passed in to the constructor.

[mikeyrf]

Also relevant from audit. Would change to something like:

address public immutable ovl;
address public immutable balancerV2Vault;

// TODO: eventually get rid of ovlWethPool storage in favor of ovlXPool approach we have with Uniswap
address public immutable ovlWethPool;

constructor(
    address _ovl,
    address _balancerV2Vault,
    address _ovlWethPool,
    uint256 _microWindow,
    uint256 _macroWindow
) OverlayV1FeedFactory(_microWindow, _macroWindow) {
    ovl = _ovl;
    balancerV2Vault = _balancerV2Vault;
    ovlWethPool = _ovlWethPool;
}

function deployFeed(
    bytes32 marketPoolId,
    address marketBaseToken,
    address marketQuoteToken,
    uint128 marketBaseAmount,
    BalancerV2Tokens.Info memory balancerV2Tokens
) external returns (address feed_) {
    // get the pool address for market tokens
    (address marketPool, ) = IBalancerV2Vault(balancerV2Vault).getPool(marketPoolId)

    // check feed doesn't already exist
    require(
        getFeed[marketPool][marketBaseToken][marketQuoteToken][marketBaseAmount] == address(0),
        "OVLV1: feed already exists"
    );

    ...

This way we revert on the withRegisteredPool() modifier Balancer has if the passed in pool ID isn't actually a valid pool

[mikeyrf]

Related to comment above. Can get rid of BalancerV2Tokens.Info passed in here since we don't need the IDs (we can determine them from your getPoolId() function) in the feed and have access to the vault in the following manner using the getVault() public method:

// get the vault from the pools
IBalancerVault vault = IBalancerPool(marketPool).getVault();
require(vault == IBalancerPool(ovlWethPool).getVault(), "OVLV1: !IBalancerVault");

// get the market and ovlWethPool IDs
bytes32 marketPoolId = getPoolId(marketPool);
bytes32 ovlWethPoolId = getPoolId(ovlWethPool);

// get the pool tokens for market and ovlWeth
(IERC20[] memory marketTokens, , ) = vault.getPoolTokens(marketPoolId)
(IERC20[] memory ovlWethTokens, , ) = vault.getPoolTokens(ovlWethPoolId)

// now do require statements for the tokens 0, 1 ...

Eliminates the need to pass in the vault address to the feed as well.

[mikeyrf]

Not sure at the moment, but there's likely a way to gas optimize this call using getPastAccumulators() instead of using getTimeWeightedAverage() (did this with Uniswap feed). I'd imagine this will be the most gas intensive part of _fetch().

Balancer's priceOracle.getTimeWeightedAverage() does a loop over queries -- which has length 3 for market pool.

Each iteration of that loop, QueryProcessor.getTimeWeightedAverage() is called, which has two internal calls to QueryProcessor.getPastAccumulator(). This results in 3 * 2 = 6 total calls to the accumulator view function.

But since our queries need past accumulator values for (now, now - microWindow, now - macroWindow, now - 2 * macroWindow), we should only have to call getPastAccumulators() a total of 4 times.

[mikeyrf]

This will return potentially incorrect results.

Should check whether ovlWethToken0 == WETH as we know reserveInWeth is in WETH and we want the ovlWeth pool price, not the market pool price, to multiply or divide that reserve number by.

Need to change to:

uint256 ovlWethPairPrice = getPairPriceOvlWeth();
if (ovlWethToken0 == WETH) {
    // if OVL is base (token1) then OVL/WETH price is #weth/#ovl
    reserve_ = reserveInWeth.divUp(ovlWethPairPrice);
} else if (ovlWethToken1 == WETH) {
    // if OVL is quote (token0) then OVL/WETH price is #ovl/#weth
    reserve_ = reserveInWeth.mulUp(ovlWethPairPrice);
} else {
    revert("OVLV1Feed: WETH not quote or base token in ovlWethPool");
}

WARNING: For Balancer token0 is the quote token for the pool, given the manner in which the oracle calculates and stores log price observations. The opposite is true for Uniswap V3 where token1 is the quote (See 6.4 in Uni V3 WP).

This means the logic above is correct for Balancer.

[mikeyrf]

In my notes I have this expression for B_0 should have the term in parantheses raised to w1 (not w0 as it is in the code comment):

B_0 = [ twav * (priceOverMicroWindow * w0 / w1) ** w1 ] ** (1 / (w_0 + w_1))

where w0 + w1 = 1

[mikeyrf]

and for B_1 I have in my notes ...

B_1 = [ twav / (priceOverMicroWindow * w_0 / w_1) ** w_0 ] ** (1 / (w_0 + w_1))

where w_0 + w_1 = 1.

[mikeyrf]

This expression for reserveInWeth_ will only be correct if marketToken1 == WETH since then we need to divide by your denominator expression. Otherwise we'd have to multiply by denominator given the notes above for calculating B_0 and B_1. So won't be correct when marketToken0 == WETH.

[mikeyrf]

Given the above + accounting for notes on B_0 and B_1, would rewrite the getReserveInWeth() function in the following manner (or something like this):

    function getReserveInWeth(uint256 twav, uint256 priceOverMicroWindow)
        public
        view
        returns (uint256 reserveInWeth_)
    {
        // Retrieve pool weights
        // Ex: a 60 WETH/40 DAI pool returns [400000000000000000 DAI, 600000000000000000 WETH]
        uint256[] memory normalizedWeights = getNormalizedWeights(marketPool);

        // WeightedPool2Tokens contract only ever has two pools
        uint256 weightToken0 = normalizedWeights[0]; // DAI
        uint256 weightToken1 = normalizedWeights[1]; // WETH
        uint256 power = uint256(1).divUp(weightToken0.add(weightToken1));

        // If marketToken0 (the base token) is WETH, solve for B0
        // If marketToken1 (the quote token) is WETH, solve for B1
        if (marketToken0 == WETH) {
            // B_0 = [ twav * (priceOverMicroWindow * w_0 / w_1) ** w_1 ] ** (1 / (w_0 + w_1))
            uint256 numerator = (priceOverMicroWindow.mulUp(weightToken0).divUp(weightToken1)).powUp(
                weightToken1
            );
            reserveInWeth_ = twav.mulUp(numerator).powUp(power);
        } else if (marketToken1 == WETH) {
            // B_1 = [ twav / (priceOverMicroWindow * w_0 / w_1) ** w_0 ] ** (1 / (w_0 + w_1))
            uint256 denominator = (priceOverMicroWindow.mulUp(weightToken0).divUp(weightToken1)).powUp(
                weightToken0
            );
            reserveInWeth_ = twav.divUp(denominator).powUp(power);
        } else {
            revert("OVLV1Feed: WETH not a market token");
        }
    }

NOTE: Please check this compiles and re-check the above code.

We can make a further simplification if we assume w_0 + w_1 = 1 forever by not raising by .powUp(power) and removing uint256 power from the code. If we do that, we should have a require() statement in the constructor that checks this is indeed the case for weights w0 and w1.

[mikeyrf]

@sicknick99 added more changes, auditing the feed. Not finished yet, but some initial issues added above.

[mikeyrf]

MIT please!

[mikeyrf]

This is still vulnerable to manipulation given any random user can call deployFeed(). And is passing in the vault address, which should be "trusted". Best place to put the "trusted" vault would be as an immutable variable stored on factory deployment.

See this for potential code change.