Omniscia Teahouse Finance Audit

TeaVaultV3Portfolio Code Style Findings

TeaVaultV3Portfolio Code Style Findings

TVV-01C: Deprecated Usage of Signature Value Literals

Description:

The referenced value literals represent function signatures which is a deprecated code style.

Example:

contracts/TeaVaultV3Portfolio.sol
717// 0xc04b8d59: bytes4(keccak256( "exactInput((bytes,address,uint256,uint256,uint256))" ))
718// 0xf28c0498: bytes4(keccak256( "exactOutput((bytes,address,uint256,uint256,uint256))" ))
719abi.encodeWithSelector(
720    TeaVaultV3Portfolio.simulateSwapViaV3RouterInternal.selector,
721    bytes4(uint32(_isExactInput ? 0xc04b8d59 : 0xf28c0498)),
722    _uniswapV3SwapRouter,
723    params
724)

Recommendation:

While the value literals have been confirmed as correct, we advise the Teahouse Finance team to import the relevant interfaces (i.e. ISwapRouter) and use the selector syntax (i.e. ISwapRouter.exactInput.selector instead of 0xc04b8d59).

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The selector syntax we have advised is now applied in the code, increasing its legibility greatly.

TVV-02C: Inefficient Emission of Captured Fees

Description:

The EntryFeeCollected event is meant to indicate what entry fees were collected, however, it is redundantly emitted when the senderNotVault variable is false.

Example:

contracts/TeaVaultV3Portfolio.sol
269uint256 totalAmount;
270bool senderNotVault = msg.sender != _feeConfig.vault;
271uint256 entryFeeAmount;
272uint256[] memory entryFeeAmounts = new uint256[](_assets.length);
273uint256 assetsLength = (totalShares == 0 ? 1 : _assets.length);
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}
297emit EntryFeeCollected(_feeConfig.vault, entryFeeAmounts, block.timestamp);

Recommendation:

We advise the event to be solely emitted if senderNotVault is true, optimizing the code's gas cost.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The EntryFeeCollected event is now solely emitted when the senderNotVault flag is true, optimizing the function's execution cost.

TVV-03C: Inefficient Emissions of Contextual Variable

Description:

The referenced events will emit the current block.timestamp redundantly as it is already attached to the transaction that each event is emitted in.

Example:

contracts/TeaVaultV3Portfolio.sol
689emit Swap(msg.sender, _srcToken, _dstToken, _swapRouter, _inputAmount, convertedAmount, block.timestamp);

Recommendation:

We advise such contextual information to not be emitted as part of the event, optimizing their gas cost while minimizing their data redundancy.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The Teahouse Finance team evaluated this optimization and opted not to apply it as they wish their off-chain software to be able to easily extract the timestamp that an event was emitted in without relying on block information.

Based on this, we consider this exhibit safely acknowledged.

TVV-04C: Inefficient On-Chain Path Calculation

Description:

The TeaVaultV3Portfolio::uniswapV3SwapViaSwapRouter function is meant to be an administrative function yet calculates an entire swap path on-chain instead of accepting the path as a direct argument of the function.

Example:

contracts/TeaVaultV3Portfolio.sol
596/// @inheritdoc ITeaVaultV3Portfolio
597function uniswapV3SwapViaSwapRouter(
598    bool _isExactInput,
599    address[] calldata _tokens,
600    uint24[] calldata _fees,
601    uint256 _deadline,
602    uint256 _amountInOrOut,
603    uint256 _amountOutOrInTolerance
604) external override onlyManager nonReentrant returns (
605    uint256 amountOutOrIn
606) {
607    address srcToken = _tokens[0];
608    address dstToken = _tokens[_tokens.length - 1];
609    bytes memory recommendedPath =_checkAndGetRecommendedPath(_isExactInput, srcToken, dstToken);
610    address _uniswapV3SwapRouter = uniswapV3SwapRouter;
611    uint256 simulatedAmount = simulateSwapViaV3Router(
612        _uniswapV3SwapRouter, _isExactInput, srcToken, recommendedPath, _amountInOrOut
613    );
614

615    bytes memory path = abi.encodePacked(_isExactInput ? srcToken : dstToken);
616    uint256 feesLength = _fees.length;
617    for (uint256 i; i < feesLength; i = i + 1) {
618        path = bytes.concat(
619            path,
620            _isExactInput ? 
621                abi.encodePacked(_fees[i], _tokens[i + 1]) : 
622                abi.encodePacked(_fees[feesLength - i - 1], _tokens[feesLength - i - 1])
623        );
624    }

Recommendation:

We advise the responsibility of constructing a valid trade path to be delegated to the manager of the portfolio, significantly reducing the on-chain gas cost of the TeaVaultV3Portfolio::uniswapV3SwapViaSwapRouter function.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The recommendation was applied as we advised, splitting the path calculation to an external and pure function that the caller of the TeaVaultV3Portfolio::uniswapV3SwapViaSwapRouter function should invoke to construct a valid _path for it.

TVV-05C: Inefficient Ownership Transfer

Description:

The OwnableUpgradeable::transferOwnership function invoked within TeaVaultV3Portfolio::initialize will redundantly ensure the caller is the owner.

Example:

contracts/TeaVaultV3Portfolio.sol
66function initialize(
67    string calldata _name,
68    string calldata _symbol,
69    uint24 _feeCap,
70    FeeConfig calldata _feeConfig,
71    address _manager,
72    ERC20Upgradeable _baseAsset,
73    ERC20Upgradeable[] calldata _assets,
74    AssetType[] calldata _assetTypes,
75    IPool _aavePool,
76    address _uniswapV3SwapRouter,
77    UniswapV3PathRecommender _pathRecommender,
78    IAssetOracle _assetOracle,
79    IAssetOracle _aaveATokenOracle,
80    IAssetOracle _teaVaultV3PairOracle,
81    Swapper _swapper,
82    address _owner
83) public initializer {
84    __UUPSUpgradeable_init();
85    __Ownable_init();
86    __ReentrancyGuard_init();
87    __ERC20_init(_name, _symbol);
88

89    SECONDS_IN_A_YEAR = 365 * 24 * 60 * 60;
90    PERCENTAGE_MULTIPLIER = 1000000;
91    FEE_CAP = _feeCap;
92    DECIMALS = 18;
93    _assignManager(_manager);
94    _setFeeConfig(_feeConfig);
95    aavePool = _aavePool;
96    uniswapV3SwapRouter = _uniswapV3SwapRouter;
97    pathRecommender = _pathRecommender;
98

99    assetOracle = _assetOracle;
100    aaveATokenOracle = _aaveATokenOracle;
101    teaVaultV3PairOracle = _teaVaultV3PairOracle;
102    swapper = _swapper;
103

104    _addAsset(_baseAsset, AssetType.Base);
105    for (uint256 i; i < _assets.length; i = i + 1) {
106        _addAsset(_assets[i], _assetTypes[i]);
107    }
108    
109    if (
110        address(_baseAsset) != _assetOracle.getBaseAsset() ||
111        address(_baseAsset) != _teaVaultV3PairOracle.getBaseAsset()
112    ) revert IAssetOracle.BaseAssetMismatch();
113

114    transferOwnership(_owner);
115    emit TeaVaultV3PortCreated(address(this), _name, _symbol);
116}

Recommendation:

We advise the internal OwnableUpgradeable::_transferOwnership function to be invoked instead, optimizing the contract's initialization cost.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The OwnableUpgradeable::_transferOwnership function is now correctly utilized, optimizing the code's initialization cost.

TVV-06C: Inefficient Removal of Array Element

Description:

The TeaVaultV3Portfolio::removeAsset function is inefficient in the way to removes assets as it will cache the asset-to-be-removed to a local variable (asset), remove it, and then proceed to set its unrelated assetType to AssetType::Null.

Example:

contracts/TeaVaultV3Portfolio.sol
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 set the entry's asset type to AssetType::Null prior to removing it from the assets array, optimizing the function's gas cost.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The Teahouse Finance team stated that the asset is actually part of a post-removal emission and thus a cache of it is required. As such, we consider the original optimization inapplicable.

TVV-07C: Inefficient Shift Operation

Description:

The TeaVaultV3Portfolio::removeAsset function will inefficiently overwrite the assets element at _index if the _index equals the assets.length - 1 value.

Example:

contracts/TeaVaultV3Portfolio.sol
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 assignment to be solely performed when _index != assets.length - 1, optimizing the code's worst-case gas cost.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The array shift operation is now performed solely when the _index to be replaced is different than the last element of the array (assets.length - 1), optimizing its gas cost.

TVV-08C: Inefficient Variable Mutability (Constant)

Description:

The referenced variables are solely assigned to value literals in the contract's TeaVaultV3Portfolio::initialize function.

Example:

contracts/TeaVaultV3Portfolio.sol
66function initialize(
67    string calldata _name,
68    string calldata _symbol,
69    uint24 _feeCap,
70    FeeConfig calldata _feeConfig,
71    address _manager,
72    ERC20Upgradeable _baseAsset,
73    ERC20Upgradeable[] calldata _assets,
74    AssetType[] calldata _assetTypes,
75    IPool _aavePool,
76    address _uniswapV3SwapRouter,
77    UniswapV3PathRecommender _pathRecommender,
78    IAssetOracle _assetOracle,
79    IAssetOracle _aaveATokenOracle,
80    IAssetOracle _teaVaultV3PairOracle,
81    Swapper _swapper,
82    address _owner
83) public initializer {
84    __UUPSUpgradeable_init();
85    __Ownable_init();
86    __ReentrancyGuard_init();
87    __ERC20_init(_name, _symbol);
88

89    SECONDS_IN_A_YEAR = 365 * 24 * 60 * 60;
90    PERCENTAGE_MULTIPLIER = 1000000;
91    FEE_CAP = _feeCap;
92    DECIMALS = 18;

Recommendation:

We advise their assignments to be relocated to their declarations and each variable's mutability to be set to constant, significantly optimizing their read-access gas cost.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

All three variables have been set as constant per our recommendation, optimizing the code's gas cost.

TVV-09C: Inefficient Variable Mutability (Immutable)

Description:

The referenced variable is solely assigned to an input argument in the contract's TeaVaultV3Portfolio::initialize function.

Example:

contracts/TeaVaultV3Portfolio.sol
66function initialize(
67    string calldata _name,
68    string calldata _symbol,
69    uint24 _feeCap,
70    FeeConfig calldata _feeConfig,
71    address _manager,
72    ERC20Upgradeable _baseAsset,
73    ERC20Upgradeable[] calldata _assets,
74    AssetType[] calldata _assetTypes,
75    IPool _aavePool,
76    address _uniswapV3SwapRouter,
77    UniswapV3PathRecommender _pathRecommender,
78    IAssetOracle _assetOracle,
79    IAssetOracle _aaveATokenOracle,
80    IAssetOracle _teaVaultV3PairOracle,
81    Swapper _swapper,
82    address _owner
83) public initializer {
84    __UUPSUpgradeable_init();
85    __Ownable_init();
86    __ReentrancyGuard_init();
87    __ERC20_init(_name, _symbol);
88

89    SECONDS_IN_A_YEAR = 365 * 24 * 60 * 60;
90    PERCENTAGE_MULTIPLIER = 1000000;
91    FEE_CAP = _feeCap;

Recommendation:

We advise its assignment to be relocated to the contract's TeaVaultV3Portfolio::constructor and its mutability to be set to immutable, optimizing its read-access gas cost significantly while remaining compatible with the TeaVaultV3Portfolio contract's upgradeable pattern.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The Teahouse Finance team stated that they do not wish to have to specify immutable variables each time they perform a contract upgrade. As such, we consider this exhibit safely acknowledged.

TVV-10C: Loop Iterator Optimizations

Description:

The linked for loops increment / decrement their iterator "safely" due to Solidity's built - in safe arithmetics (post-0.8.X).

Example:

contracts/TeaVaultV3Portfolio.sol
105for (uint256 i; i < _assets.length; i = i + 1) {

Recommendation:

We advise the increment / decrement operations to be performed in an unchecked code block as the last statement within each for loop to optimize their execution cost.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The referenced loop iterator increment statements have been relocated at the end of each respective for loop's body and have been unwrapped in an unchecked code block, optimizing their gas cost.

TVV-11C: Redundant Local Variables

Description:

The referenced local variables are assigned to a value literal (type(uint256).max) which is inefficient as it will reserve memory when unnecessary.

Example:

contracts/TeaVaultV3Portfolio.sol
705uint256 UINT256_MAX = type(uint256).max;
706ERC20Upgradeable(_srcToken).approve(_uniswapV3SwapRouter, UINT256_MAX);
707

708SwapRouterGenericParams memory params = SwapRouterGenericParams({
709    path: _path,
710    recipient: address(this),
711    deadline: UINT256_MAX,
712    amountInOrOut: _amountInOrOut,
713    amountOutOrInTolerance: _isExactInput ? 0 : UINT256_MAX
714});

Recommendation:

We advise the value literal to simply be placed in all references of each local variable, optimizing each code segment's gas cost.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The exhibit has been fully alleviated as the UINT256_MAX variables are no longer present in the codebase.

TVV-12C: Redundant Maintenance of Total Amounts

Description:

The totalAmount variable is redundantly incremented on each deposit while being solely utilized in a non-zero check.

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}
297emit EntryFeeCollected(_feeConfig.vault, entryFeeAmounts, block.timestamp);
298// if shares is too low such that no assets is needed, do not mint
299if (totalAmount == 0) revert InvalidShareAmount();

Recommendation:

Given that it does not provide any useful data in its current state, we advise the code to instead use a bool that is written to once indicating that at least one asset was processed.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The variable has been converted to a bool as advised that is set to true every time, optimizing the code's gas cost as a result.

TVV-13C: Repetitive Value Literal

Description:

The linked value literal is repeated across the codebase multiple times.

Example:

contracts/TeaVaultV3Portfolio.sol
192if (_feeConfig.decayFactor >= 2 ** 128) revert InvalidFeeRate();

Recommendation:

We advise it to be set to a constant variable instead optimizing the legibility of the codebase.

Alleviation (302b96f324):

While the referenced variable has been relocated to a DECAT_FACTOR_100_PERCENT named constant, all instances referenced by the exhibit have not been properly replaced rendering it partially addressed.

Alleviation (1dc136da87):

All instances have been properly replaced by the aptly renamed DECAY_FACTOR_100_PERCENT, addressing this exhibit in full.

Prev

TeaVaultV3PairOracle.sol (TVP-C)

Next

TeaVaultV3PortfolioHelper.sol (TVH-C)

On this page