MetavisorManagedVault Manual Review Findings

MMV-01M: Inexistent Conformity to Checks-Effects-Interactions Pattern

Type	Severity	Location
Standard Conformity		MetavisorManagedVault.sol:L141-L142, L147

Description:

The reference statements within withdraw do not conform to the Checks-Effects-Interactions (CEI) pattern as the funds to be withdrawn are distributed to the user (potentially in native format) prior to their shares being burned.

Impact:

While not an active attack vector, the code could become susceptible to a re-entrancy attack in a future version should the nonReentrant modifier be removed or inadequately applied to other functions. The CEI pattern should be adhered to in all function implementations of the contract regardless of its re-entrancy protection measures.

Example:

Copy
117function withdraw(
118    uint256 shares,
119    bool _asEth
120) external nonReentrant returns (uint256 amount0, uint256 amount1) {
121    if (shares == 0) {
122        revert Errors.InvalidWithdraw();
123    }
124
125    uint256 tSupply = totalSupply();
126
127    uint256 fees0;
128    uint256 fees1;
129
130    (amount0, amount1, fees0, fees1) = UniswapInteractionHelper.burnLiquidity(
131        pool,
132        ticksData,
133        shares,
134        tSupply,
135        false
136    );
137
138    emit FeeClaimed(fees0, fees1);
139    transferProtocolFees(fees0, fees1);
140
141    amount0 += FullMath.mulDiv(_balance0() - amount0, shares, tSupply);
142    amount1 += FullMath.mulDiv(_balance1() - amount1, shares, tSupply);
143
144    _transferSend(token0, msg.sender, amount0, _asEth);
145    _transferSend(token1, msg.sender, amount1, _asEth);
146
147    _burn(msg.sender, shares);
148    emit Withdrawn(msg.sender, shares, amount0, amount1);
149
150    if (totalSupply() != 0) {
151        _compound(false);
152    }
153}

Recommendation:

While the codebase is protected via the nonReentrant modifier, the code should still conform to the CEI pattern as a matter of best practice. We advise the _burn statement to be relocated before the _transferSend invocations, ensuring that the user has had their shares burned prior to receiving their assets as is logically expected of the system. The _compound mechanism can remain after the fund distribution as it is inconsequential to the security of the project.

Alleviation:

The _burn statement was relocated prior to the outward distribution of funds to the msg.sender, ensuring that the contract's state is correct when external calls are performed.

MMV-02M: Unsafe Casting Operations

Type	Severity	Location
Mathematical Operations		MetavisorManagedVault.sol:L240-L241

Description:

The rescale function will calculate the proportionate amountToSwap via a percentage calculation whose result is unsafely cast to an int256 data type.

Impact:

Based on the implementation of the MetavisorManagedVault, the FullMath::mulDiv calculation cannot exceed the int256 bound unless the swapPercentage defined in MetavisorRegistry exceeds the DENOMINATOR value, a trait presently permitted in the constructor of the contract.

Example:

Copy
239int256 amountToSwap = zeroForOne
240    ? int256(FullMath.mulDiv(amount0Available - amount0, swapPercentage, DENOMINATOR))
241    : int256(FullMath.mulDiv(amount1Available - amount1, swapPercentage, DENOMINATOR));

Recommendation:

We advise the casting operations to be performed safely by ensuring that the value of the interim FullMath::mulDiv calculation does not exceed the upper bound of the int256 data type (type(int256).max). We should note that Solidity's built-in safe arithmetic in pragma versions 0.8.X does not accommodate for casting operations and as such they need to be manually protected.

Alleviation:

The swapPercentage value has been limited to values less than the DENOMINATOR and this exhibit has been indirectly alleviated as a result.

MMV-03M: Inexistent Validation of Input / Output Amounts

Type	Severity	Location
Logical Fault		MetavisorManagedVault.sol:L107, L141-L142

Description:

The deposit and withdraw workflows of the contract do not adequately validate the actual amounts utilized as input / extracted as output, rendering the user's transaction fully susceptible to slippage attacks.

Impact:

The user's interactions with the Metavisor vault are prone to any form of slippage related attack (such as sandwich attacks) as they have no control over what price / conversion rate they will deposit / withdraw their funds at.

Example:

Copy
80/*
81 ** Core Interactions
82 */
83function deposit(
84    uint256 amount0,
85    uint256 amount1
86) external payable nonReentrant returns (uint256 shares) {
87    if (amount0 != 0 || amount1 != 0) {
88        uint256 tSupply = totalSupply();
89
90        if (tSupply != 0) {
91            _compound(true); // Ensures only collectable fee is used for share computation
92        }
93
94        shares = UniswapInteractionHelper.computeShares(
95            pool,
96            amount0,
97            amount1,
98            _balance0(),
99            _balance1(),
100            tSupply,
101            ticksData
102        );
103
104        _transferReceive(token0, msg.sender, address(this), amount0);
105        _transferReceive(token1, msg.sender, address(this), amount1);
106
107        UniswapInteractionHelper.mintLiquidity(pool, ticksData, amount0, amount1);
108
109        _mint(msg.sender, shares);
110
111        emit Deposited(msg.sender, shares, amount0, amount1);
112    } else {
113        revert Errors.InvalidDeposit();
114    }
115}
116
117function withdraw(
118    uint256 shares,
119    bool _asEth
120) external nonReentrant returns (uint256 amount0, uint256 amount1) {
121    if (shares == 0) {
122        revert Errors.InvalidWithdraw();
123    }
124
125    uint256 tSupply = totalSupply();
126
127    uint256 fees0;
128    uint256 fees1;
129
130    (amount0, amount1, fees0, fees1) = UniswapInteractionHelper.burnLiquidity(
131        pool,
132        ticksData,
133        shares,
134        tSupply,
135        false
136    );
137
138    emit FeeClaimed(fees0, fees1);
139    transferProtocolFees(fees0, fees1);
140
141    amount0 += FullMath.mulDiv(_balance0() - amount0, shares, tSupply);
142    amount1 += FullMath.mulDiv(_balance1() - amount1, shares, tSupply);
143
144    _transferSend(token0, msg.sender, amount0, _asEth);
145    _transferSend(token1, msg.sender, amount1, _asEth);
146
147    _burn(msg.sender, shares);
148    emit Withdrawn(msg.sender, shares, amount0, amount1);
149
150    if (totalSupply() != 0) {
151        _compound(false);
152    }
153}

Recommendation:

We advise a minIn / minOut argument to be introduced in the relevant functions that contains two uint256 variables and represents the minimum input / minimum output the user is willing to provide / acquire.

Alleviation:

The deposit and withdraw functions have both had two new Min-suffixed arguments representing the minimum amount of asset 0 / asset 1 the user is willing to provide / extract from the pair, ensuring that they deposit funds to the pair at the ratio they desire and that the withdrawal they perform results in the assets they expect.

MMV-04M: Slippage Prone Compounding Mechanism

Type	Severity	Location
Logical Fault		MetavisorManagedVault.sol:L274-L296

Description:

The _compound mechanism applies no access control and can be arbitraged via a slippage-based attack (such as sandwich attacks) to extract a portion of the fees to-be-compounded at an unfavourable rate.

Impact:

The compounding mechanism employed by the contract is fully susceptible to slippage attacks and may even supply liquidity to a ticksData that is vastly out of the current tick range given that vault may have some delay between being able to be rescaled and actually being rescaled.

Example:

Copy
271/*
272 ** Helpers
273 */
274function _compound(bool _collect) internal {
275    if (_collect) {
276        (, , uint256 fees0, uint256 fees1) = UniswapInteractionHelper.burnLiquidity(
277            pool,
278            ticksData,
279            0,
280            totalSupply(),
281            false
282        );
283
284        emit FeeClaimed(fees0, fees1);
285        transferProtocolFees(fees0, fees1);
286    }
287
288    (uint256 new0, uint256 new1) = UniswapInteractionHelper.mintLiquidity(
289        pool,
290        ticksData,
291        _balance0(),
292        _balance1()
293    );
294
295    emit Compounded(new0, new1);
296}

Recommendation:

The compounding mechanism _compound and all functions that utilize it need to provide an adequate security measure against slippage attacks. To achieve this, we advise the implementation and integration of a TWAP mechanism that disallows sharp price movements in the time preceding the transaction, ensuring that the contract behaves under normal price conditions.

Alleviation:

All functions of the MetavisorManagedVault now apply a TWAP-based modifier (validateTwap), restricting the price movements of the pair within the past few seconds within a per-vault specified threshold. The Metavisor team has stated that they aim to fine-tune the TWAP intervals and thresholds per vault with a default configuration of a 10% movement permitted within 60 seconds which we consider adequate for volatile assets.