TeaVaultV3PortfolioHelper Manual Review Findings

TVH-01M: Inexplicable Implementations of Uniswap V3 Interactions

Type	Severity	Location
Standard Conformity		TeaVaultV3PortfolioHelper.sol:L304-L320, L322-L330, L332-L338

Description:

The TeaVaultV3PortfolioHelper contract contains implementations (i.e. TeaVaultV3PortfolioHelper::depositV3Pair) that integrate with their own system (ITeaVaultV3Pair) as well as generic Uniswap V3 integrating implementations (i.e. TeaVaultV3PortfolioHelper::v3PairDeposit) which are identical.

Impact:

While vulnerabilities have been identified in separate exhibits, we consider the overall discrepancy to be unknown given that there is no rationale behind their introduction within the code.

Example:

Copy
303/// @inheritdoc ITeaVaultV3PortfolioHelper
304function v3PairDeposit(
305    address _v3pair,
306    uint256 _shares,
307    uint256 _amount0Max,
308    uint256 _amount1Max
309) external payable onlyInMulticall returns (uint256 depositedAmount0, uint256 depositedAmount1) {
310    ITeaVaultV3Pair v3pair = ITeaVaultV3Pair(_v3pair);
311    IERC20 token0 = IERC20(v3pair.assetToken0());
312    IERC20 token1 = IERC20(v3pair.assetToken1());        
313    token0.safeApprove(_v3pair, type(uint256).max);
314    token1.safeApprove(_v3pair, type(uint256).max);
315    (depositedAmount0, depositedAmount1) = v3pair.deposit(_shares, _amount0Max, _amount1Max);
316
317    // since v3pair is specified by the caller, it's safer to remove all allowances after depositing
318    token0.safeApprove(_v3pair, 0);
319    token1.safeApprove(_v3pair, 0);
320}

Recommendation:

We advise the latter to be omitted as they are presently insecurely implemented as evidenced in a separate exhibit and are unnecessary given that the ITeaVaultV3Pair integrations are interchangeable with those of Uniswap V3.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The Teahouse Finance team evaluated these implementations and specified that they are indeed meant for different operations depending on the Teahouse Finance ecosystem contract interacted with.

To further illustrate this, they have introduced checks to the TeaVaultV3PortfolioHelper::v3PairDeposit and TeaVaultV3PortfolioHelper::aaveSupply functions that properly make them distinct.

We consider this exhibit alleviated per the Teahouse Finance team's latest actions as the functions are no longer identical in operation.

TVH-02M: Deprecated Approval Operation

Type	Severity	Location
Standard Conformity		TeaVaultV3PortfolioHelper.sol:L150, L156, L169, L206, L230, L231, L235, L236, L249, L250, L270, L271, L313, L314, L318, L319

Description:

OpenZeppelin has officially deprecated usage of the SafeERC20::safeApprove function due to its non-standard nature.

Impact:

The contract presently relies on the assumption that all non-zero approvals performed are done so on tokens with a zero approval beforehand, a trait which can at times be broken via complex combination of functions.

Additionally, this type of check is generally unnecessary for contracts such as the TeaVaultV3PortfolioHelper, rendering the SafeERC20::forceApprove function the best choice for this type of implementation.

Example:

Copy
142/// @inheritdoc ITeaVaultV3PortfolioHelper
143function deposit(
144    uint256 _shares
145) external payable onlyInMulticall returns (uint256[] memory amounts) {
146    ERC20Upgradeable[] memory tokens = ITeaVaultV3Portfolio(vault).getAssets();
147    uint256 tokensLength = tokens.length;
148
149    for(uint256 i; i < tokensLength; i++) {
150        tokens[i].safeApprove(vault, type(uint256).max);
151    }
152    amounts = ITeaVaultV3Portfolio(vault).deposit(_shares);
153
154    // since vault is specified by the caller, it's safer to remove all allowances after depositing
155    for(uint256 i; i < tokensLength; i++) {
156        tokens[i].safeApprove(vault, 0);
157    }
158
159    // send the resulting shares to the caller
160    IERC20(vault).safeTransfer(msg.sender, _shares);
161}

Recommendation:

We advise the code to instead make use of functions such as SafeERC20::forceApprove which are guaranteed to execute safely and efficiently.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

All referenced SafeERC20::safeApprove operations have been properly replaced by SafeERC20::forceApprove invocations, ensuring that the approvals are performed as expected and in an up-to-date manner.

TVH-03M: Inexistent Prevention of Default Vault Value

Type	Severity	Location
Input Sanitization		TeaVaultV3PortfolioHelper.sol:L49

Description:

The TeaVaultV3PortfolioHelper::multicall function can be re-entered if the vault specified as input is the address(0x1) special flag, a behaviour we consider incorrect.

Impact:

While a re-entrancy would indeed be permitted resulting in a nested multicall, only the innermost multicall will be able to conduct operations on a valid vault as all other multicall operations will have a vault of address(0x1).

As such, we consider this exhibit to be of minimal impact.

Example:

Copy
46/// @inheritdoc ITeaVaultV3PortfolioHelper
47function multicall(
48    VaultType _vaultType,
49    address _vault,
50    address[] calldata _tokens,
51    uint256[] calldata _amounts,
52    bytes[] calldata _data
53) external payable returns (bytes[] memory results) {
54    if (vault != address(0x1)) {
55        revert NestedMulticall();
56    }
57
58    vault = _vault;

Recommendation:

We advise the function to prevent the input _vault from being specified as address(0x1), preventing these types of re-entrancies from occurring.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The _vault argument is now sanitized to prevent it from being the special address(0x1) flag, preventing re-entrancies and thus alleviating this exhibit.

TVH-04M: Insecure Uniswap V3 LP Provision

Type	Severity	Location
Logical Fault		TeaVaultV3PortfolioHelper.sol:L303-L320

Description:

The TeaVaultV3PortfolioHelper::v3PairDeposit function will perform a deposit to the v3pair, however, the acquired funds from the v3pair will remain unused in contrast to TeaVaultV3PortfolioHelper::depositV3Pair and can potentially be exploited by later calls.

Impact:

We consider this to be a medium severity vulnerability as a proper call to TeaVaultV3PortfolioHelper::multicall can account for those funds (i.e. by supplying the token as input with a 0 amount or including a TeaVaultV3PortfolioHelper::refundTokens payload) but the most likely scenario is the token's omission, leading to loss of funds.

Example:

Copy
303/// @inheritdoc ITeaVaultV3PortfolioHelper
304function v3PairDeposit(
305    address _v3pair,
306    uint256 _shares,
307    uint256 _amount0Max,
308    uint256 _amount1Max
309) external payable onlyInMulticall returns (uint256 depositedAmount0, uint256 depositedAmount1) {
310    ITeaVaultV3Pair v3pair = ITeaVaultV3Pair(_v3pair);
311    IERC20 token0 = IERC20(v3pair.assetToken0());
312    IERC20 token1 = IERC20(v3pair.assetToken1());        
313    token0.safeApprove(_v3pair, type(uint256).max);
314    token1.safeApprove(_v3pair, type(uint256).max);
315    (depositedAmount0, depositedAmount1) = v3pair.deposit(_shares, _amount0Max, _amount1Max);
316
317    // since v3pair is specified by the caller, it's safer to remove all allowances after depositing
318    token0.safeApprove(_v3pair, 0);
319    token1.safeApprove(_v3pair, 0);
320}

Recommendation:

We advise the code to properly transfer any acquired funds to the msg.sender as they will not be accounted for by the TeaVaultV3PortfolioHelper::multicall function.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The code of TeaVaultV3PortfolioHelper::v3PairDeposit now ensures the asset interacted with is part of the vault's portfolio, guaranteeing that they are refunded at the end of a multi-call's execution.

As such, we consider this exhibit properly dealt with.

TVH-05M: Insecure Uniswap V3 LP Withdrawal

Type	Severity	Location
Logical Fault		TeaVaultV3PortfolioHelper.sol:L337

Description:

The TeaVaultV3PortfolioHelper::v3PairWithdrawMax function will inexplicably specify the minimum outputs to the ITeaVaultV3Pair::withdraw call as 0, permitting arbitrageurs to exploit such transactions and acquire a profit by diminishing their return.

Impact:

The vulnerability described will lead to loss of funds due to arbitrage, resulting in a moderate impact with a moderate likelihood of being exploitable.

Example:

Copy
332/// @inheritdoc ITeaVaultV3PortfolioHelper
333function v3PairWithdrawMax(
334    address _v3pair
335) external payable onlyInMulticall returns (uint256 withdrawnAmount0, uint256 withdrawnAmount1) {
336    uint256 shares = IERC20(_v3pair).balanceOf(address(this));
337    (withdrawnAmount0, withdrawnAmount1) = ITeaVaultV3Pair(_v3pair).withdraw(shares, 0, 0);
338}

Recommendation:

We advise the code to properly relay user input to the ITeaVaultV3Pair::withdraw call, allowing the original TeaVaultV3PortfolioHelper::multicall caller to supply proper minimum outputs expected for their maximum withdrawal operation, the maximum part being already guaranteed by the IERC20::balanceOf measurement of the _v3pair balance.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

Minimum outputs have been introduced as input to the TeaVaultV3PortfolioHelper::multicall function thereby ensuring that withdrawal operations (as well as potentially other token balances) can be validated in relation to their minimum outputs.

Based on these changes, we consider this exhibit properly alleviated.