sherlock-admin4 commented 5 months ago

GiuseppeDeLaZara

high

`mTOFT` can be forced to receive the wrong ERC20 leading to token lockup

Summary

Due to Stargate's functionality of swapping one token on the source chain to another token on the destination chain, it is possible to force mTOFT to receive the wrong ERC20 token leading to token lockup.

Vulnerability Detail

Stargate allows for swapping between different tokens. These are usually correlated stablecoins. They are defined as Stargate Chains Paths inside the docs: https://stargateprotocol.gitbook.io/stargate/developers/stargate-chain-paths.

To give an example, a user can:

Provide USDC on Ethereum and receive USDT on Avalanche.
Provide USDC on Avalanche and receive USDT on Arbitrum.
etc.

This can also be observed by just playing around with the Stargate UI: https://stargate.finance/transfer.

The Balancer.sol contract initializes the connected OFTs through the initConnectedOFT function. This function is only callable by the admin and he specifies the src and dst pool ids. PoolIds refer to a specific StargatePool that holds the underlying asset(USDC, USDT, etc.): https://stargateprotocol.gitbook.io/stargate/developers/pool-ids.

The issue here is that poolIds are not enforced during the rebalancing process. As it can be observed the bytes memory _ercData is not checked for its content.

## Balancer.sol

function _sendToken(
        address payable _oft,
        uint256 _amount,
        uint16 _dstChainId,
        uint256 _slippage,
>>>        bytes memory _data
    ) private {
        address erc20 = ITOFT(_oft).erc20();
        if (IERC20Metadata(erc20).balanceOf(address(this)) < _amount) {
            revert ExceedsBalance();
        }
        {
>>>            (uint256 _srcPoolId, uint256 _dstPoolId) = abi.decode(_data, (uint256, uint256));
            _routerSwap(_dstChainId, _srcPoolId, _dstPoolId, _amount, _slippage, _oft, erc20);
        }
    }

It is simply decoded and passed as is.

This is a problem and imagine the following scenario:

A Gelato bot calls the rebalance method for mTOFT that has USDC as erc20 on Ethereum.
The bot encodes the ercData so srcChainId = 1 pointing to USDC but dstChainId = 2 pointing to USDT on Avalanche.
Destination mTOFT is fetched from connectedOFTs and points to the mTOFT with USDC as erc20 on Avalanche.
Stargate will take USDC on Ethereum and provide USDT on Avalanche.
mTOFT with USDC as underlying erc20 on Avalanche will receive USDT token and it will remain lost as the balance of the mTOFT contract.

As this is a clear path for locking up wrong tokens inside the mTOFT contract, it is a critical issue.

Impact

The impact of this vulnerability is critical. It allows for locking up wrong tokens inside the mTOFT contract causing irreversible loss of funds.

Code Snippet

https://github.com/sherlock-audit/2024-02-tapioca/blob/dc2464f420927409a67763de6ec60fe5c028ab0e/TapiocaZ/contracts/Balancer.sol#L293

Tool used

Manual Review

Recommendation

The initConnectedOFT function should enforce the poolIds for the src and dst chains.The rebalance function should just fetch these saved values and use them.


@@ -164,14 +176,12 @@ contract Balancer is Ownable {
      * @param _dstChainId the destination LayerZero id
      * @param _slippage the destination LayerZero id
      * @param _amount the rebalanced amount
-     * @param _ercData custom send data
      */
     function rebalance(
         address payable _srcOft,
         uint16 _dstChainId,
         uint256 _slippage,
-        uint256 _amount,
-        bytes memory _ercData
+        uint256 _amount
     ) external payable onlyValidDestination(_srcOft, _dstChainId) onlyValidSlippage(_slippage) {
         {
@@ -188,13 +204,13 @@ contract Balancer is Ownable {
             if (msg.value == 0) revert FeeAmountNotSet();
             if (_isNative) {
                 if (disableEth) revert SwapNotEnabled();
                 _sendNative(_srcOft, _amount, _dstChainId, _slippage);
             } else {
-                _sendToken(_srcOft, _amount, _dstChainId, _slippage, _ercData);
+                _sendToken(_srcOft, _amount, _dstChainId, _slippage);
             }

@@ -221,7 +237,7 @@ contract Balancer is Ownable {
      * @param _dstOft the destination TOFT address
      * @param _ercData custom send data
      */
-    function initConnectedOFT(address _srcOft, uint16 _dstChainId, address _dstOft, bytes memory _ercData)
+    function initConnectedOFT(address _srcOft, uint256 poolId, uint16 _dstChainId, address _dstOft, bytes memory _ercData)
         external
         onlyOwner
     {
@@ -231,10 +247,8 @@ contract Balancer is Ownable {
         bool isNative = ITOFT(_srcOft).erc20() == address(0);
         if (!isNative && _ercData.length == 0) revert PoolInfoRequired();

-        (uint256 _srcPoolId, uint256 _dstPoolId) = abi.decode(_ercData, (uint256, uint256));
-
         OFTData memory oftData =
-            OFTData({srcPoolId: _srcPoolId, dstPoolId: _dstPoolId, dstOft: _dstOft, rebalanceable: 0});
+            OFTData({srcPoolId: poolId, dstPoolId: poolId, dstOft: _dstOft, rebalanceable: 0});

         connectedOFTs[_srcOft][_dstChainId] = oftData;
         emit ConnectedChainUpdated(_srcOft, _dstChainId, _dstOft);

     function _sendToken(
         address payable _oft,
         uint256 _amount,
         uint16 _dstChainId,
-        uint256 _slippage,
-        bytes memory _data
+        uint256 _slippage
     ) private {
         address erc20 = ITOFT(_oft).erc20();
         if (IERC20Metadata(erc20).balanceOf(address(this)) < _amount) {
             revert ExceedsBalance();
-        }
+            }
         {
-            (uint256 _srcPoolId, uint256 _dstPoolId) = abi.decode(_data, (uint256, uint256));
-            _routerSwap(_dstChainId, _srcPoolId, _dstPoolId, _amount, _slippage, _oft, erc20);
+            _routerSwap(_dstChainId, _amount, _slippage, _oft, erc20);
         }
     }

     function _routerSwap(
         uint16 _dstChainId,
-        uint256 _srcPoolId,
-        uint256 _dstPoolId,
         uint256 _amount,
         uint256 _slippage,
         address payable _oft,
         address _erc20
     ) private {
         bytes memory _dst = abi.encodePacked(connectedOFTs[_oft][_dstChainId].dstOft);
+        uint256 poolId = connectedOFTs[_oft][_dstChainId].srcPoolId;
         IERC20(_erc20).safeApprove(address(router), _amount);
         router.swap{value: msg.value}(
             _dstChainId,
-            _srcPoolId,
-            _dstPoolId,
+            poolId,
+            poolId,
             payable(this),
             _amount,
             _computeMinAmount(_amount, _slippage),

Admin is trusted but you can optionally add additional checks inside the initConnectedOFT function to ensure that the poolIds are correct for the src and dst mTOFTs.

sherlock-admin4 commented 5 months ago

The protocol team fixed this issue in PR/commit https://github.com/Tapioca-DAO/TapiocaZ/pull/175.

dmitriia commented 5 months ago

Escalate While the impact is clearly high/critical, the probability of this looks to be low/very low as rebalance() is permissioned, so what is described here is a keeper/configuration mistake and there looks to be no way for this to be exploited by an outside attacker. Why user mistakes with the very same full fund loss impact (e.g. #112, #132) are discarded, while keeper mistake isn't? Notice that per contest terms all protocol actors are trusted:

Are the admins of the protocols your contracts integrate with (if any) TRUSTED or RESTRICTED?

TRUSTED

Is the admin/owner of the protocol/contracts TRUSTED or RESTRICTED?

TRUSTED

In the same time, as this is more deep in nature compared to common mistakes, I would say medium severity can be applicable.

sherlock-admin2 commented 5 months ago

Escalate While the impact is clearly high/critical, the probability of this looks to be low/very low as rebalance() is permissioned, so what is described here is a keeper/configuration mistake and there looks to be no way for this to be exploited by an outside attacker. Why user mistakes with the very same full fund loss impact (e.g. #112, #132) are discarded, while keeper mistake isn't? Notice that per contest terms all protocol actors are trusted:
Are the admins of the protocols your contracts integrate with (if any) TRUSTED or RESTRICTED?

TRUSTED

Is the admin/owner of the protocol/contracts TRUSTED or RESTRICTED?

TRUSTED
In the same time, as this is more deep in nature compared to common mistakes, I would say medium severity can be applicable.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

windhustler commented 5 months ago

The escalation comment is comparing apples to oranges. User mistakes are completely different than the critical severity attack path described here.

rebalance function was meant to be called by an automation system to offload manually calling the function. The function definitely shouldn’t exclude basic input parameters validation which the report above highlights.

Since when are off-chain agents responsible for validating inputs and the smart contract security being offloaded to the off-chain server?

Moreover, most automation systems are either partly or on the road to being decentralized where anyone can become an operator. So there is a clear path for a malicious actor to exploit this.

The issue is not some obscure, low-likelihood attack path but rather a straightforward way of irreversibly losing all the rebalanced amount.

If we look at: https://github.com/sherlock-audit/2024-02-tapioca-judging/issues/87 ,a hypothetical token with different decimals across chains was accepted as “medium likelihood”, although such a token isn't accepted as collateral right and there is only a small probability a token with such properties will be added in the future.

dmitriia commented 5 months ago

The lack of any configuration check in a permissioned function usually do not account for anything above medium as the only way for it to cause any damage is operator's mistake.

87 is about user-facing flow.

windhustler commented 5 months ago

It's a bit more nuanced than that. Based on the number of issues in the Balancer contract it seems that the team has overlooked several scenarios. When it comes to off-chain agents, most of them are either fully or on their way to becoming decentralized:

https://keep3r.network/ Anyone can become a keeper.
https://forum.gelato.network/discussion/11360-node-operator-staking-wave-1 Gelato network plans are to also become fully decentralized with time.

So, this issue also highlights the importance of validating inputs with functions meant to be called by these agents.

But, as always I leave it to the judge to decide on the severity.

https://github.com/sherlock-audit/2024-02-tapioca-judging/issues/87 is about Governance setting tokens with different decimals across chains as collateral in the future versions of Tapioca. As these tokens are very uncommon and even less likely to be used as collateral the likelihood of this occurring is very low. The report also doesn't mention a concrete token, so we can assume it's a hypothetical one.

dmitriia commented 5 months ago

Anyone cannot run rebalance(), its use is fixed to rebalancer actor, one address. Permissionless rebalancing setups tend to constitute high severity surfaces on their own. Any decentralized mechanics, i.e. when actors can be random, but, for example, have something at stake, need to be examined for incentives. Say when NPV of the attack payout is greater than actor's stake in a keeper system, then it's a rough equivalent of permissionless setup with lower attack payoff, and might be exploited.

87 is about a miss in the logic (not converting the input), this is about not checking the configuration of the permissioned call.

windhustler commented 5 months ago

You have proof I pasted above of how the automation system is decentralized or getting decentralized. The contest README also states: "External issues/integrations that would affect Tapioca, should be considered."

https://github.com/sherlock-audit/2024-02-tapioca-judging/issues/87 And an exploit that occurs with a token that doesn't exist yet.

cvetanovv commented 5 months ago

I think escalation is correct because rebalance() is restricted and trusted. The only reason I see a chance for it to remain a valid issue is that in the Readme we have "External issues/integrations that would affect Tapioca, should be considered."

But that's exactly why it falls into the Medium category. The function is not for everyone and there are too many conditions. Let's look at the Sherlock documentation for Medium severity: "Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained."

So I think it's fair to accept the escalation and downgrade to Medium.

cvetanovv commented 5 months ago

Planning to accept the escalation and make this issue a Medium.

windhustler commented 5 months ago

@cvetanovv Thanks for taking a look. I'd still appreciate it if @Czar102 could take a look at this one. I acknowledge the permissioned nature of the rebalance function, but as I've highlighted this can be ambiguous under certain conditions and it's handing over the security to an automation system. Also, consider my points that other issues with very low likelihood https://github.com/sherlock-audit/2024-02-tapioca-judging/issues/87 were judged as high severity.

Thanks!

Evert0x commented 4 months ago

Result: Medium Unique

sherlock-admin3 commented 4 months ago

Escalations have been resolved successfully!

Escalation status:

dmitriia: accepted

sherlock-audit / 2024-02-tapioca-judging

GiuseppeDeLaZara - `mTOFT` can be forced to receive the wrong ERC20 leading to token lockup #70

`mTOFT` can be forced to receive the wrong ERC20 leading to token lockup

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

87 is about user-facing flow.

87 is about a miss in the logic (not converting the input), this is about not checking the configuration of the permissioned call.

sherlock-audit / 2024-02-tapioca-judging

GiuseppeDeLaZara - `mTOFT` can be forced to receive the wrong ERC20 leading to token lockup #70

mTOFT can be forced to receive the wrong ERC20 leading to token lockup

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

87 is about user-facing flow.

87 is about a miss in the logic (not converting the input), this is about not checking the configuration of the permissioned call.

`mTOFT` can be forced to receive the wrong ERC20 leading to token lockup