Platform: Code4rena
Start Date: 16/11/2021
Pot Size: $50,000 ETH
Total HM: 11
Participants: 17
Period: 7 days
Judge: LSDan
Total Solo HM: 8
Id: 49
League: ETH
Rank: 1/17
Findings: 4
Award: $9,592.02
🌟 Selected for report: 3
🚀 Solo Findings: 1
🌟 Selected for report: cmichel
1.6578 ETH - $7,659.53
cmichel
The OverlayV1UniswapV3Market.fetchPricePoint tries to compute the market depth in OVL terms as marketLiquidity (in ETH) / ovlPrice (in ETH per OVL).
To get the market liquidity in ETH (and not the other token pair), it uses the ethIs0 boolean.
_marketLiquidity = ethIs0 ? ( uint256(_liquidity) << 96 ) / _sqrtPrice : FullMath.mulDiv(uint256(_liquidity), _sqrtPrice, X96);
However, ethIs0 boolean refers to the ovlFeed, whereas the _liquidity refers to the marketFeed, and therefore the ethIs0 boolean has nothing to do with the market feed where the liquidity is taken from:
// in constructor, if token0 is eth refers to ovlFeed ethIs0 = IUniswapV3Pool(_ovlFeed).token0() == _eth; // in fetchPricePoint, _liquidity comes from different market feed ( _ticks, _liqs ) = IUniswapV3Pool(marketFeed).observe(_secondsAgo); _marketLiquidity = ethIs0 ? ( uint256(_liquidity) << 96 ) / _sqrtPrice : FullMath.mulDiv(uint256(_liquidity), _sqrtPrice, X96);
If the ovlFeed and marketFeed do not have the same token position for the ETH pair (ETH is either token 0 or token 1 for both pairs), then the market liquidity & depth is computed wrong (inverted).
For example, the OverlayV1Market.depth() function will return a wrong depth which is used in the market cap computation.
It seems that marketFeed.token0() == WETH should be used in fetchPricePoint to compute the liquidity instead of ovlFeed.token0() == WETH.
#0 - commercium-sys
2021-12-03T16:27:24Z
Yeah, was aware of this, just hadn't finalized it in the code as of yet.
cmichel
The OverlayToken has a transferMint and transferBurn function which is supposed to act like a transfer followed by a mint/burn.
However, a mint/burn updates the _totalSupply (see _mint/_burn) but these functions do not.
The transferMint and transferBurn should have the same semantics as a transfer followed by a mint/burn.
#0 - mikeyrf
2021-12-06T23:46:18Z
duplicate #59
cmichel
The Overlayv1Mothership.adjustGlobalParams function allows setting a fee and feeBurn that is greater than 100%.
Validate that fee and feeBurn is less than ONE = 1e18.
#0 - mikeyrf
2021-12-06T23:29:29Z
duplicate #77 - bounds on governance params
🌟 Selected for report: cmichel
cmichel
The OverlayV1UniswapV3Market contract assumes that one of the tokens of _ovlFeed is ETH but does not check it in the constructor:
constructor( address _mothership, address _ovlFeed, address _marketFeed, address _quote, address _eth, uint128 _baseAmount, uint256 _macroWindow, uint256 _microWindow, uint256 _priceFrameCap ) OverlayV1Market ( _mothership ) OverlayV1Comptroller ( _microWindow ) OverlayV1OI ( _microWindow ) OverlayV1PricePoint ( _priceFrameCap ) { // immutables eth = _eth; // could be that token1 is not ETH either ethIs0 = IUniswapV3Pool(_ovlFeed).token0() == _eth; // ... }
If token0 is not ETH, then it assumes token1 is ETH but never validates this assumption.
This could lead to wrong market liquidity and prices calculations if an _ovlFeed is supplied that is not actually the OVL/ETH feed.
Check that (token0 == OVL && token1 == WETH) || (token1 == OVL && token0 == WETH) for _ovlFeed.
🌟 Selected for report: cmichel
cmichel
The OverlayV1UniswapV3Market.constructor does not verify that the marcoWindow > microWindow but the code implicitly uses this assumption when computing the TWAPs.
Validate that macroWindow > microWindow in the constructor.