Omniscia Teahouse Finance Audit

TeaVaultV3Portfolio Manual Review Findings

TeaVaultV3Portfolio Manual Review Findings

TVV-01M: Non-Standard Gap Size Specification

Description:

The __gap variable is meant to adhere to the OpenZeppelin upgradeability standard, however, its size has been incorrectly defined. Specifically, the size of the __gap variable should equate the value of 50 minus all storage slots presently reserved by the contract.

Example:

contracts/TeaVaultV3Portfolio.sol
813uint256[35] private __gap;

Recommendation:

We advise the __gap variable's length to be corrected to 50 - 18 = 32 instead of 35 as the contract presently reserves 18 storage slots. Should the immutable and constant optimizations advised be applied, the length should instead be corrected to 34.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The __gap variable's length has been corrected to 34 as the immutable and constant optimizations have been applied to the codebase.

TVV-02M: Discrepant Management of Fee Times

Description:

The TeaVaultV3Portfolio is discrepant in the way it handles time updates for its management and performance fees. Specifically, it will update the management fee regardless of whether any shares were actually collected (i.e. due to a short interval) whilst the performance fee time will solely be updated when non-zero fees have been acquired.

Impact:

The system will currently forfeit management fees if they are frequently captured whilst performance fees will always be captured regardless of invocation frequency.

Example:

contracts/TeaVaultV3Portfolio.sol
356function _collectManagementFee(
357    uint256 _totalShares,
358    FeeConfig memory _feeConfig
359) internal returns (
360    uint256 collectedShares
361) {
362    uint256 timeDiff = block.timestamp - lastCollectManagementFee;
363    if (timeDiff > 0) {
364        unchecked {
365            uint256 feeTimesTimediff = _feeConfig.managementFee * timeDiff;
366            uint256 denominator = (
367                PERCENTAGE_MULTIPLIER * SECONDS_IN_A_YEAR > feeTimesTimediff ?
368                    PERCENTAGE_MULTIPLIER * SECONDS_IN_A_YEAR - feeTimesTimediff :
369                    1
370            );
371            collectedShares = _totalShares.mulDiv(feeTimesTimediff, denominator, MathUpgradeable.Rounding.Up);
372        }
373

374        if (collectedShares > 0) {
375            _mint(_feeConfig.vault, collectedShares);
376            emit ManagementFeeCollected(_feeConfig.vault, collectedShares, block.timestamp);
377        }
378

379        lastCollectManagementFee = block.timestamp;
380    }
381}
382

383/// @inheritdoc ITeaVaultV3Portfolio
384function collectPerformanceFee() external override nonReentrant returns (uint256 collectedShares) {
385    ERC20Upgradeable[] memory _assets = assets;
386

387    return _collectPerformanceFee(
388        totalSupply(),
389        _calculateAssetsValue(_assets, _calculateTotalAmounts(_assets), _getAssetsTwap(_assets)),
390        feeConfig
391    );
392}
393

394function _collectPerformanceFee(
395    uint256 _totalShares,
396    uint256 _totalValue,
397    FeeConfig memory _feeConfig
398) internal returns (
399    uint256 collectedShares
400) {
401    // calculate and transfer the unlocked performance fee from the reserve
402    uint256 timeDiff = block.timestamp - lastCollectPerformanceFee;
403    if (performanceFeeReserve > 0 && timeDiff > 0) {
404        collectedShares = performanceFeeReserve.mulDiv(
405            (1 << 128) - power128(feeConfig.decayFactor, timeDiff),
406            1 << 128,
407            MathUpgradeable.Rounding.Up
408        );
409        if (collectedShares > 0) {
410            performanceFeeReserve -= collectedShares;
411            _transfer(address(this), feeConfig.vault, collectedShares);
412            lastCollectPerformanceFee = block.timestamp;
413            emit PerformanceFeeCollected(feeConfig.vault, collectedShares, block.timestamp);
414        }
415    }

Recommendation:

We advise this discrepancy to be corrected, either ensuring that both fees have their time updated when fees are actually acquired or that the time is updated regardless of whether fees have been collected, the former of which we expect to be desirable behaviour.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The TeaVaultV3Portfolio::_collectPerformanceFee function was updated to assign its lastCollectPerformanceFee value at all times regardless of whether a fee was actually captured.

As such, we consider this exhibit alleviated as the performance and management fees have become consistent in their application.

TVV-03M: Improper Fee Acquisition Methodology

Description:

The TeaVaultV3Portfolio system will acquire deposit fees by extracting an additional value on top of the expected deposit from the caller. This is non-standard behaviour and will lead to significant UX problems given that users will have to "over-approve" their assets to the system in order to interact with it.

Impact:

The current deposit fee acquisition mechanism will lead to failed transactions, difficulty in on-chain integration, and a generally bad user experience for users and integrators alike.

Example:

contracts/TeaVaultV3Portfolio.sol
274for (uint256 i; i < assetsLength; i = i + 1) {
275    if (shareAmounts[i] > 0) {
276        _assets[i].safeTransferFrom(msg.sender, address(this), shareAmounts[i]);
277        totalAmount = totalAmount + shareAmounts[i];
278        
279        // collect entry fee for users
280        // do not collect entry fee for fee recipient
281        if (senderNotVault) {
282            entryFeeAmount = shareAmounts[i].mulDiv(
283                _feeConfig.entryFee,
284                PERCENTAGE_MULTIPLIER,
285                MathUpgradeable.Rounding.Up
286            );
287

288            if (entryFeeAmount > 0) {
289                totalAmount = totalAmount + entryFeeAmount;
290                _assets[i].safeTransferFrom(msg.sender, _feeConfig.vault, entryFeeAmount);
291                entryFeeAmounts[i] = entryFeeAmount;
292            }
293        }
294        depositedAmounts[i] = shareAmounts[i] + entryFeeAmount;
295    }
296}

Recommendation:

We advise the fee to either be imposed on the input amounts, or to be imposed on the output _shares, either of which we consider an adequate resolution to this exhibit.

Alleviation (302b96f324):

The Teahouse Finance team specified that the user would not over-approve in this case as the calculations are performed before the deposit is actually done.

We would like to note that on-chain contracts would not be able to reliably integrate with the system, and the over-approval in our exhibit refers to an approval beyond the actual deposit rather than a lingering approval from the user to the system.

Based on the aforementioned facts, we advise the Teahouse Finance team to revisit this exhibit.

Alleviation (1dc136da87):

The Teahouse Finance team revisited this exhibit and opted to retain the current behaviour in place, rendering it acknowledged.

TVV-04M: Improper Order of Performance Fee Evaluation

Description:

The TeaVaultV3Portfolio::_collectPerformanceFee function is currently incorrect as it will attempt to capture the performance fee from a pool of shares that are calculated after the fee is captured.

Coupled with the fact that the performance fee's time variable is updated solely when fees are captured, the system becomes inefficient as it will have to be re-invoked to actually capture the fees reserved in the latter part of the function's body.

Impact:

The system is presently inefficient and would lead to untapped performance fees at any given time.

Example:

contracts/TeaVaultV3Portfolio.sol
394function _collectPerformanceFee(
395    uint256 _totalShares,
396    uint256 _totalValue,
397    FeeConfig memory _feeConfig
398) internal returns (
399    uint256 collectedShares
400) {
401    // calculate and transfer the unlocked performance fee from the reserve
402    uint256 timeDiff = block.timestamp - lastCollectPerformanceFee;
403    if (performanceFeeReserve > 0 && timeDiff > 0) {
404        collectedShares = performanceFeeReserve.mulDiv(
405            (1 << 128) - power128(feeConfig.decayFactor, timeDiff),
406            1 << 128,
407            MathUpgradeable.Rounding.Up
408        );
409        if (collectedShares > 0) {
410            performanceFeeReserve -= collectedShares;
411            _transfer(address(this), feeConfig.vault, collectedShares);
412            lastCollectPerformanceFee = block.timestamp;
413            emit PerformanceFeeCollected(feeConfig.vault, collectedShares, block.timestamp);
414        }
415    }
416

417    if (_totalShares != 0) {
418        if (_totalValue > highWaterMark) {
419            uint256 increasedReserveValue = (_totalValue - highWaterMark).mulDiv(
420                _feeConfig.performanceFee,
421                PERCENTAGE_MULTIPLIER,
422                MathUpgradeable.Rounding.Up
423            );
424            uint256 increasedReserve = _totalShares.mulDiv(
425                increasedReserveValue,
426                _totalValue - increasedReserveValue,
427                MathUpgradeable.Rounding.Up
428            );
429            performanceFeeReserve = performanceFeeReserve + increasedReserve;
430            _mint(address(this), increasedReserve);
431            highWaterMark = _totalValue;
432        }
433        else {
434            uint256 decreasedReserveValue = (highWaterMark - _totalValue).mulDiv(
435                _feeConfig.performanceFee,
436                PERCENTAGE_MULTIPLIER
437            );
438            uint256 decreasedReserve = _totalShares.mulDiv(
439                decreasedReserveValue,
440                _totalValue + decreasedReserveValue
441            );
442            decreasedReserve = decreasedReserve > performanceFeeReserve ? performanceFeeReserve : decreasedReserve;
443            performanceFeeReserve = performanceFeeReserve - decreasedReserve;
444            _burn(address(this), decreasedReserve);
445        }
446    }
447}

Recommendation:

We advise the statements to be re-ordered, calculating the performanceFeeReserve prior to actually capturing the fees.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The Teahouse Finance team specified that the described behaviour by the exhibit is by design as the performance fee is meant to be unlocked over time rather than immediately.

As such, we consider the code correct and adhering to the contract's business requirements rendering this exhibit invalid.

TVV-05M: Insufficient Validation of Oracle Compatibility

Description:

The TeaVaultV3Portfolio::initialize function will validate that the base asset of the _assetOracle and _teaVaultV3PairOracle contracts match the one in TeaVaultV3Portfolio, however, the base asset of the aaveATokenOracle is never validated.

Impact:

The TeaVaultV3Portfolio can presently be initialized with an aaveATokenOracle that utilizes a different base asset which we consider incorrect.

Example:

contracts/TeaVaultV3Portfolio.sol
99assetOracle = _assetOracle;
100aaveATokenOracle = _aaveATokenOracle;
101teaVaultV3PairOracle = _teaVaultV3PairOracle;
102swapper = _swapper;
103

104_addAsset(_baseAsset, AssetType.Base);
105for (uint256 i; i < _assets.length; i = i + 1) {
106    _addAsset(_assets[i], _assetTypes[i]);
107}
108

109if (
110    address(_baseAsset) != _assetOracle.getBaseAsset() ||
111    address(_baseAsset) != _teaVaultV3PairOracle.getBaseAsset()
112) revert IAssetOracle.BaseAssetMismatch();

Recommendation:

We advise it to be validated as well, ensuring all oracles are synchronized regarding the _baseAsset they rely on.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The base asset of the _aaveATokenOracle is now validated as well in the referenced if conditional, alleviating this exhibit in full.

TVV-06M: Improper Assumptions of Asset Maintenance

Description:

The TeaVaultV3Portfolio::addAsset and TeaVaultV3Portfolio::removeAsset functions are flawed and will not behave as expected in a production environment of the TeaVaultV3Portfolio. Specifically, a newly added asset via TeaVaultV3Portfolio::addAsset will never have a non-zero balance as the TeaVaultV3Portfolio::_calculateTotalAmounts function would yield zero for it.

On the other hand, a removal of an asset via TeaVaultV3Portfolio::removeAsset will evaluate whether a non-zero balance of the asset-to-be-removed exists in the contract and prevent execution in such a case. This permits hijackers to transmit a negligible amount (i.e. 1 wei) to the contract of an asset being removed to keep it within the assets list forever.

Impact:

The current mechanisms in place for maintaining the assets supported by the TeaVaultV3Portfolio are insufficient and can be hijacked, forcing unwanted assets to remain part of the portfolio.

Example:

contracts/TeaVaultV3Portfolio.sol
129/// @inheritdoc ITeaVaultV3Portfolio
130function addAsset(ERC20Upgradeable _asset, AssetType _assetType) external override onlyOwner {
131    _addAsset(_asset, _assetType);
132}
133

134function _addAsset(ERC20Upgradeable _asset, AssetType _assetType) internal {
135    // base asset must be the first to be added
136    if (_assetType == AssetType.Null || _assetType >= AssetType.End) revert InvalidAssetType();
137    if (_assetType == AssetType.Base && assets.length != 0) revert BaseAssetCannotBeAdded();
138    if (assetType[_asset] != AssetType.Null) revert AssetAlreadyAdded();
139    if (_assetType == AssetType.Atomic && !assetOracle.isOracleEnabled(address(_asset))) revert OracleNotEnabled();
140    if (_assetType == AssetType.AToken && !aaveATokenOracle.isOracleEnabled(address(_asset))) revert OracleNotEnabled();
141    if (_assetType == AssetType.TeaVaultV3Pair && !teaVaultV3PairOracle.isOracleEnabled(address(_asset))) revert OracleNotEnabled();
142    
143    assets.push(_asset);
144    assetType[_asset] = _assetType;
145

146    emit AssetAdded(address(_asset), block.timestamp);
147}
148

149/// @inheritdoc ITeaVaultV3Portfolio
150function removeAsset(uint256 _index) external override onlyOwner {
151    if (assets[_index].balanceOf(address(this)) != 0) revert AssetBalanceNotZero();
152    if (assetType[assets[_index]] == AssetType.Base) revert BaseAssetCannotBeRemoved();
153

154    ERC20Upgradeable asset = assets[_index];
155    assets[_index] = assets[assets.length - 1];
156    assets.pop();
157    assetType[asset] = AssetType.Null;
158

159    emit AssetRemoved(address(asset), block.timestamp);
160}

Recommendation:

We advise the code to be revised as the assets list in its current state cannot be dynamically updated. Example remediations include converting the asset to be removed to the supported assets of the system, and supporting the inclusion of a new asset solely if existing balance is converted to it.

Alleviation (302b96f324):

The Teahouse Finance team stated that they wish to retain the dynamic adjustment of the assets list in place due to their business requirements.

We would like to reiterate that this exhibit does not state that such functionality should be removed, it instead states that the functionality is presently impossible and easy to sabotage.

As such, we advise the Teahouse Finance team to revisit this exhibit.

Alleviation (a337648498):

The Teahouse Finance team re-evaluated this exhibit and opted to introduce an on-chain mechanism whereby a swap is performed before the removal of the asset. This mechanism while valid is presently insufficient as it will rely on the TeaVaultV3Portfolio::swapAndRemoveAsset input amount (_inputAmount).

To ensure the function can be reliably utilized, we advise the code to use the full balance of the contract in the respective asset.

Alleviation (1dc136da87):

The code was updated to properly calculate the _inputAmount of the swap dynamically in the TeaVaultV3Portfolio::swapAndRemoveAsset flow, however, the TeaVaultV3Portfolio::removeAsset flow was updated in this commit as well and will perform a Uniswap V3 withdrawal with no slippage control thereby rendering the full withdrawn amount susceptible to sandwich attacks.

We advise proper slippage values to be supplied to the ITeaVaultV3Pair::withdraw call, ensuring that the withdrawn amounts are properly associated with the LP units burned.

Alleviation (3a0cc9c15a):

The code was updated to properly accept minimum outputs for all assets in the vault that are validated after the swap of the asset-to-be-removed but prior to its removal, thereby ensuring that any swap that occurred has abode by the expected slippage thresholds configured by the caller.

As such, we consider this exhibit fully alleviated.

TVV-07M: Potential Hijack of High Water Mark Initialization

Description:

The TeaVaultV3Portfolio::deposit function relies on an empty balance of the contract to initialize the highWaterMark which can be trivially hijacked, causing the highWaterMark to remain zero until the first time a performance fee is captured at an unfair evaluation.

Impact:

By donating supported assets to the portfolio directly, the highWaterMark will not be initialized with the first deposit and as such the first TeaVaultV3Portfolio::_collectPerformanceFee invocation after the first deposit will capture a performance fee as if the contract went from 0 to the depositedValue.

Example:

contracts/TeaVaultV3Portfolio.sol
262if (totalValue == 0) {
263    highWaterMark = depositedValue;
264}
265else {
266    highWaterMark = highWaterMark.mulDiv(totalValue + depositedValue, totalValue);
267}

Recommendation:

We advise the code to instead check if the highWaterMark is 0, ensuring that its value is initialized regardless of the system's state.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The code was updated to correctly initialize the value of the highWaterMark, evaluating whether its 0 and either initializing it to the depositedValue or updating it per the business requirements of the system.

As such, we consider this exhibit fully alleviated.

Prev

TeaVaultV3PairOracle.sol (TVP-M)

Next

TeaVaultV3PortfolioHelper.sol (TVH-M)

On this page