sherlock-admin3 commented 6 months ago

GiuseppeDeLaZara

medium

Gas parameters for Stargate swap are hardcoded leading to stuck messages

Summary

The dstGasForCall for transferring erc20s through Stargate is hardcoded to 0 in the Balancer contract leading to sgReceive not being called during Stargate swap. As a consequence, the sgReceive has to be manually called to clear the cachedSwapLookup mapping, but this can be DoSed due to the fact that the mTOFT::sgReceive doesn't validate any of its parameters. This can be exploited to perform a long-term DoS attack.

Vulnerability Detail

Gas parameters for Stargate

Stargate Swap allows the caller to specify the:

dstGasForCall which is the gas amount forwarded while calling the sgReceive on the destination contract.
dstNativeAmount and dstNativeAddr which is the amount and address where the native token is sent to.

Inside the Balancer.sol contract, the dstGasForCall is hardcoded to 0. The dstGasForCall gets forwarded from Stargate Router into the Stargate Bridge contract.

    function swap(
        uint16 _chainId,
        uint256 _srcPoolId,
        uint256 _dstPoolId,
        address payable _refundAddress,
        Pool.CreditObj memory _c,
        Pool.SwapObj memory _s,
>>>>>>        IStargateRouter.lzTxObj memory _lzTxParams, 
        bytes calldata _to,
        bytes calldata _payload
    ) external payable onlyRouter {
>>>>>>        bytes memory payload = abi.encode(TYPE_SWAP_REMOTE, _srcPoolId, _dstPoolId, _lzTxParams.dstGasForCall, _c, _s, _to, _payload);
        _call(_chainId, TYPE_SWAP_REMOTE, _refundAddress, _lzTxParams, payload);
    }

    function _call(
        uint16 _chainId,
        uint8 _type,
        address payable _refundAddress,
        IStargateRouter.lzTxObj memory _lzTxParams,
        bytes memory _payload
    ) internal {
        bytes memory lzTxParamBuilt = _txParamBuilder(_chainId, _type, _lzTxParams);
        uint64 nextNonce = layerZeroEndpoint.getOutboundNonce(_chainId, address(this)) + 1;
        layerZeroEndpoint.send{value: msg.value}(_chainId, bridgeLookup[_chainId], _payload, _refundAddress, address(this), lzTxParamBuilt);
        emit SendMsg(_type, nextNonce);
    }

It gets encoded inside the payload that is sent through the LayerZero message. The payload gets decoded inside the Bridge::lzReceive on destination chain. And dstGasForCall is forwarded to the sgReceive function:

## Bridge.sol

 function lzReceive(
        uint16 _srcChainId,
        bytes memory _srcAddress,
        uint64 _nonce,
        bytes memory _payload
    ) external override {
        if (functionType == TYPE_SWAP_REMOTE) {
            (
                ,
                uint256 srcPoolId,
                uint256 dstPoolId,
>>>>>                uint256 dstGasForCall,
                Pool.CreditObj memory c,
                Pool.SwapObj memory s,
                bytes memory to,
                bytes memory payload
            ) = abi.decode(_payload, (uint8, uint256, uint256, uint256, Pool.CreditObj, Pool.SwapObj, bytes, bytes));

If it is zero like in the Balancer.sol contract or its value is too small the sgReceive will fail, but the payload will be saved in the cachedSwapLookup mapping. At the same time the tokens are transferred to the destination contract, which is the mTOFT. Now anyone can call the sgReceive manually through the clearCachedSwap function:

    function clearCachedSwap(
        uint16 _srcChainId,
        bytes calldata _srcAddress,
        uint256 _nonce
    ) external {
        CachedSwap memory cs = cachedSwapLookup[_srcChainId][_srcAddress][_nonce];
        require(cs.to != address(0x0), "Stargate: cache already cleared");
        // clear the data
        cachedSwapLookup[_srcChainId][_srcAddress][_nonce] = CachedSwap(address(0x0), 0, address(0x0), "");
        IStargateReceiver(cs.to).sgReceive(_srcChainId, _srcAddress, _nonce, cs.token, cs.amountLD, cs.payload);
    }

Although not the intended behavior there seems to be no issue with erc20 token sitting on the mTOFT contract for a shorter period of time.

sgReceive

This leads to the second issue. The sgReceive function interface specifies the chainId, srcAddress, and token.

chainId is the layerZero chainId of the source chain. In their docs referred to endpointId: https://layerzero.gitbook.io/docs/technical-reference/mainnet/supported-chain-ids
srcAddress is the address of the source sending contract
token is the address of the token that was sent to the destination contract.

In the current implementation, the sgReceive function doesn't check any of these parameters. In practice this means that anyone can specify the mTOFT address as the receiver and initiate Stargate Swap from any chain to the mTOFT contract.

In conjunction with the first issue, this opens up the possibility of a DoS attack.

Let's imagine the following scenario:

Rebalancing operation needs to be performed between mTOFT on Ethereum and Avalanche that hold USDC as the underlying token.
Rebalancing is initiated from Ethereum but the sgReceive on Avalanche fails and 1000 USDCs are sitting on mTOFT contract on Avalanche.
A griever noticed this and initiated Stargate swap from Ethereum to Avalanche for 1 USDT specifying the mTOFT contract as the receiver.
This is successful and now mTOFT has 1 USDT but 999 USDC as the griever's transaction has called the sgRecieve function that pushed 1 USDC to the TOFTVault.
As a consequence, the clearCachedSwap function fails because it tries to transfer the original 1000 USDC.

    function sgReceive(uint16, bytes memory, uint256, address, uint256 amountLD, bytes memory) external payable {
        if (msg.sender != _stargateRouter) revert mTOFT_NotAuthorized();

        if (erc20 == address(0)) {
            vault.depositNative{value: amountLD}();
        } else {
>>>>>            IERC20(erc20).safeTransfer(address(vault), amountLD); // amountLD is the original 1000 USDC
        }
    }

The only solution here is to manually transfer that 1 USDC to the mTOFT contract and try calling the clearCachedSwap again.
The griever can repeat this process multiple times.

Impact

Hardcoding the dstGasCall to 0 in conjuction with not checking the sgReceive parameters opens up the possibility of a long-term DoS attack.

Code Snippet

Tool used

Manual Review

Recommendation

The dstGasForCall shouldn't be hardcoded to 0. It should be a configurable value that is set by the admin of the Balancer contract.

Take into account that this value will be different for different chains.

For instance, Arbitrum has a different gas model than Ethereum due to its specific precompiles: https://docs.arbitrum.io/arbos/gas.

The recommended solution is:

 contract Balancer is Ownable {
     using SafeERC20 for IERC20;

+    mapping(uint16 => uint256) internal sgReceiveGas;

+    function setSgReceiveGas(uint16 eid, uint256 gas) external onlyOwner {
+        sgReceiveGas[eid] = gas;
+    }
+
+    function getSgReceiveGas(uint16 eid) internal view returns (uint256) {
+        uint256 gas = sgReceiveGas[eid];
+        if (gas == 0) revert();
+        return gas;
+    }
+
-    IStargateRouterBase.lzTxObj({dstGasForCall: 0, dstNativeAmount: 0, dstNativeAddr: "0x0"}),
+    IStargateRouterBase.lzTxObj({dstGasForCall: getSgReceiveGas(_dstChainId), dstNativeAmount: 0, dstNativeAddr: "0x0"}),

cryptotechmaker commented 6 months ago

Low/Informational/ It's not a required feature. This allows you to airdrop some native tokens to a destination

sherlock-admin4 commented 6 months ago

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

HollaDieWaldfee100 commented 6 months ago

Escalate

The sponsor's comment is misleading. He is referencing the dstNativeAmount and dstNativeAddr which can be left as 0, as this functionality is not needed. The whole report talks about the dstGasForCall that is hardcoded to 0. This will lead to sgReceive not being called and open up the possibility of a DoS attack vector on the receiving side. The report also highlights the gas differences between chains and the importance of properly setting dstGasForCall per destination chain. Recommendations were implemented in the Tapioca-DAO/TapiocaZ#177. Based on all the arguments this should be a valid medium severity issue.

sherlock-admin2 commented 6 months ago

Escalate

The sponsor's comment is misleading. He is referencing the dstNativeAmount and dstNativeAddr which can be left as 0, as this functionality is not needed. The whole report talks about the dstGasForCall that is hardcoded to 0. This will lead to sgReceive not being called and open up the possibility of a DoS attack vector on the receiving side. The report also highlights the gas differences between chains and the importance of properly setting dstGasForCall per destination chain. Recommendations were implemented in the Tapioca-DAO/TapiocaZ#177. Based on all the arguments this should be a valid medium severity issue.

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.

Tendency001 commented 6 months ago

Hardcoding zero gas actually triggers a revert on the same chain down the logic in RelayerV2::_getPrices

        // decoding the _adapterParameters - reverts if type 2 and there is no dstNativeAddress
        require(_adapterParameters.length == 34 || _adapterParameters.length > 66, "Relayer: wrong _adapterParameters size");
        uint16 txType;
        uint extraGas;
        assembly {
            txType := mload(add(_adapterParameters, 2))
            extraGas := mload(add(_adapterParameters, 34))
        }
        require(extraGas > 0, "Relayer: gas too low");

The call never gets delivered to the destination chain to introduce the said DOS

windhustler commented 6 months ago

This call does get delivered to the destination chain. The dstGasForCall is the gas passed to the sgReceive, and the base gas is a configuration inside Stargate:

    function _txParamBuilder(
        uint16 _chainId,
        uint8 _type,
        IStargateRouter.lzTxObj memory _lzTxParams
    ) internal view returns (bytes memory) {
        bytes memory lzTxParam;
        address dstNativeAddr;
        {
            bytes memory dstNativeAddrBytes = _lzTxParams.dstNativeAddr;
            assembly {
                dstNativeAddr := mload(add(dstNativeAddrBytes, 20))
            }
        }

>>>        uint256 totalGas = gasLookup[_chainId][_type].add(_lzTxParams.dstGasForCall);
        if (_lzTxParams.dstNativeAmount > 0 && dstNativeAddr != address(0x0)) {
>>>            lzTxParam = txParamBuilderType2(totalGas, _lzTxParams.dstNativeAmount, _lzTxParams.dstNativeAddr);
        } else {
>>>            lzTxParam = txParamBuilderType1(totalGas);
        }

        return lzTxParam;
    }

You can see that dstGasForCall is simply added to the totalGas. In other words, Stargate will always deliver the message but if you hardcode dstGasForCall to 0 sgReceive will revert.

Setting dstGasForCall to 0 would be a strange configuration parameter if it would disallow sending messages.

Tendency001 commented 6 months ago

You are right. Great find 🫡

maarcweiss commented 6 months ago

@windhustler should we also use the set gas receiver stuff on the following PR, correct?: https://github.com/Tapioca-DAO/TapiocaZ/pull/174/files

windhustler commented 6 months ago

Hey, yes you need to use the appropriate dstGasForCall here as well. You can reuse the _sgReceiveGas value from here: https://github.com/Tapioca-DAO/TapiocaZ/pull/177/files.

cvetanovv commented 6 months ago

While it is a valid report, the main root is hardcoded dstGasCall to 0. And the same root vulnerability was found in the "Pashov Audit Group" audit - H-07. According to Sherlock's rules, this makes the report invalid.

@nevillehuang what do you think?

windhustler commented 6 months ago

@cvetanovv I've just checked H-07. It has several false/inaccurate claims which is probably the reason why Tapioca team hasn't fixed this:

It claims if dstGasForCall == 0, a fee will be charged based on the default 200k gas, i.e. sgRecieve on the destination will be called with 200k gas. This is not correct, see https://github.com/sherlock-audit/2024-02-tapioca-judging/issues/72#issuecomment-2034713451. The 200k gas is the default value if you are sending layer zero messages with gas set to 0. Using Stargate is different. dstGasForCall is exclusively used as gas passed for sgReceive.
Based on the false assumptions it derives the impact of underpaying/overpaying Stargate fees which is quite different than the impact this report claims.
It doesn't make the distinction between setting different gas configurations for different chains.

cvetanovv commented 6 months ago

After doing some research I agree with @windhustler comment. So, I plan to accept the escalation and make the issue a valid Medium.

nevillehuang commented 6 months ago

I think I agree with medium severity, unless @cryptotechmaker would like to clarify the above comment here

Evert0x commented 5 months ago

Result: Medium Unique

sherlock-admin3 commented 5 months ago

Escalations have been resolved successfully!

Escalation status:

HollaDieWaldfee100: accepted

sherlock-audit / 2024-02-tapioca-judging

GiuseppeDeLaZara - Gas parameters for Stargate swap are hardcoded leading to stuck messages #72

Gas parameters for Stargate swap are hardcoded leading to stuck messages

Summary

Vulnerability Detail

Gas parameters for Stargate

sgReceive

Impact

Code Snippet

Tool used

Recommendation