code-423n4 / 2024-01-decent-findings

3 stars 3 forks source link

Stargate fee should use the function `quoteLayerZeroFee()` instead of having an arbitrary parameter. #226

Closed c4-bot-1 closed 9 months ago

c4-bot-1 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/bridge_adapters/StargateBridgeAdapter.sol#L170

Vulnerability details

Impact

Fees paid to Stargate may not be accurate, may result in failing function.

Proof of Concept

In StargateBridgeAdapter, when calling router.swap, it carries an arbitrary msg.value.

    function callBridge(
        uint256 amt2Bridge,
        uint256 dstChainId,
        bytes memory bridgePayload,
        bytes calldata additionalArgs,
        address payable refund
    ) private {
>       router.swap{value: msg.value}(
            getDstChainId(additionalArgs), //lzBridgeData._dstChainId, // send to LayerZero chainId

This msg.value is calculated in UTB.callBridge().

    function callBridge(
        uint256 amt2Bridge,
        uint bridgeFee,
        BridgeInstructions memory instructions
    ) private returns (bytes memory) {
        bool native = approveAndCheckIfNative(instructions, amt2Bridge);
        return
            IBridgeAdapter(bridgeAdapters[instructions.bridgeId]).bridge{
                value: bridgeFee + (native ? amt2Bridge : 0)
            }(

which is set by the user in UTB.bridgeAndExecute().

    function bridgeAndExecute(
        BridgeInstructions calldata instructions,
>       FeeStructure calldata fees,
        bytes calldata signature
    )
        public
        payable
        retrieveAndCollectFees(fees, abi.encode(instructions, fees), signature)
        returns (bytes memory)
    {
        (
            uint256 amt2Bridge,
            BridgeInstructions memory updatedInstructions
        ) = swapAndModifyPostBridge(instructions);
>       return callBridge(amt2Bridge, fees.bridgeFee, updatedInstructions);
    }

In LayerZero Docs, it is adviced to use quoteLayerZeroFee() to get the fee required to call swap(). It is unsure how the fees are currently calculated now, but if the fees paid is inaccurate then the function will fail.

Reference: https://stargateprotocol.gitbook.io/stargate/developers/cross-chain-swap-fee

Tools Used

Recommended Mitigation Steps

Call router.quoteLayerZeroFee() to get the estimate of fees to pay.

Assessed type

Context

c4-pre-sort commented 9 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 9 months ago

raymondfam marked the issue as primary issue

raymondfam commented 9 months ago

Informational code refactoring involving out-of-scope contract.

alex-ppg commented 9 months ago

As the fee is supplied by the user, using the on-chain mechanism to measure it would be a nice-to-have feature rather than a security vulnerability. The submission would be graded QA (NC) which would not warrant a reward (i.e. C grade) and thus I am setting it as invalid.

c4-judge commented 9 months ago

alex-ppg marked the issue as unsatisfactory: Invalid

cryptostaker2 commented 9 months ago

Hi @alex-ppg,

This issue is a problem because the user will not know how much gas to set to pay for the stargate transfer, which will result in failed transactions.

Stargate Docs:

The fee ensures the cross chain message is paid for.

Reference: https://stargateprotocol.gitbook.io/stargate/developers/cross-chain-swap-fee

This is a similar finding, which marks the problem as M:

https://solodit.xyz/issues/m-01-use-quotelayerzerofee-instead-of-sending-all-native-asset-balance-as-gas-fee-for-swap-call-pashov-none-mugen-markdown

Giving another example, this protocol tries to calculate its own quoteLayerZeroFee(), but underestimates the fee because one component is not factored in:

https://solodit.xyz/issues/trst-m-10-mozbridge-underestimates-gas-for-sending-of-moz-messages-trust-security-none-mozaic-archimedes-markdown_

This shows that calculating the wrong amount of fees is an issue as well. The easiest way to get the fee is to use the given stargate method.

Thanks!

alex-ppg commented 9 months ago

Hey @cryptostaker2, I cannot access the referenced links as they request a login.

The second example as described is invalid as it states a fee was not accounted for; given that the user supplies it we can assume they have accounted for all relevant fees.

The first example title says "instead of sending all native asset balance" which is not what occurs in this instance. In this instance, the user supplies it, and per the relevant SC verdict this issue cannot be more than a QA flaw if at all valid.