code-423n4 / 2024-02-tapioca-findings

1 stars 1 forks source link

Hardcoded `FeeNative` can cause relaying to fail is `messagingFee` changes #87

Open c4-bot-9 opened 5 months ago

c4-bot-9 commented 5 months ago

Lines of code

https://github.com/Tapioca-DAO/tap-token/blob/050e666142e61018dbfcba64d295f9c458c69700/contracts/tokens/TapTokenReceiver.sol#L194-L196

Vulnerability details

Impact

Network or Config changes can cause fee.nativeFee to be insufficient, making xChain Messages revert indefinitely.

These fees could change, meaning that some messages could fail to be relayed for sendParam.fee.nativeFee

Code Snippet

https://github.com/Tapioca-DAO/tap-token/blob/050e666142e61018dbfcba64d295f9c458c69700/contracts/tokens/TapTokenReceiver.sol#L194-L196

            TapTokenSender(rewardToken_).sendPacket{
                value: claimTwTapRewardsMsg_.sendParam[sendParamIndex].fee.nativeFee /// @audit This can likely fail
            }(claimTwTapRewardsMsg_.sendParam[sendParamIndex], bytes(""));

For messages for which the Computed Fee on srcChain ends up being different from the required fee on dstChain, messages may fail due to insufficient fee

Because fees are hardcoded on the srcChain call, any change in the fee will cause the messages to be stuck indefinitely

POC

L0 Code

Following the L0 V2 docs: https://layerzero.gitbook.io/docs/evm-guides/contract-standards/estimating-message-fees

We can check a basic version of how messages can be priced (hardcoded values)

https://github.com/LayerZero-Labs/LayerZero-v2/blob/142846c3d6d51e3c2a0852c41b4c2b63fcda5a0a/protocol/contracts/messagelib/SimpleMessageLib.sol#L71-L92

    function send(
        Packet calldata _packet,
        bytes memory _options,
        bool _payInLzToken
    ) external onlyEndpoint returns (MessagingFee memory fee, bytes memory encodedPacket, bytes memory options) {
        encodedPacket = PacketV1Codec.encode(_packet);

        options = _options.length == 0 ? defaultOption : _options;
        _handleMessagingParamsHook(encodedPacket, options);

        fee = MessagingFee(nativeFee, _payInLzToken ? lzTokenFee : 0);
    }

    // ------------------ onlyOwner ------------------
    function setDefaultOption(bytes memory _defaultOption) external onlyOwner {
        defaultOption = _defaultOption;
    }

    function setMessagingFee(uint256 _nativeFee, uint256 _lzTokenFee) external onlyOwner {
        nativeFee = _nativeFee;
        lzTokenFee = _lzTokenFee;
    }

And a more realistic example, in which prices are based on a premium of gas used + a variable cost due to network costs

https://github.com/LayerZero-Labs/LayerZero-v2/blob/142846c3d6d51e3c2a0852c41b4c2b63fcda5a0a/messagelib/contracts/ExecutorFeeLib.sol#L106-L175

function _decodeExecutorOptions(
        bool _v1Eid,
        uint64 _baseGas,
        uint128 _nativeCap,
        bytes calldata _options
    ) internal pure returns (uint256 dstAmount, uint256 totalGas) {
        if (_options.length == 0) {
            revert Executor_NoOptions();
        }

        uint256 cursor = 0;
        bool ordered = false;
        totalGas = _baseGas;

        uint256 lzReceiveGas;
        while (cursor < _options.length) {
            (uint8 optionType, bytes calldata option, uint256 newCursor) = _options.nextExecutorOption(cursor);
            cursor = newCursor;

            if (optionType == ExecutorOptions.OPTION_TYPE_LZRECEIVE) {
                (uint128 gas, uint128 value) = ExecutorOptions.decodeLzReceiveOption(option);

                // endpoint v1 does not support lzReceive with value
                if (_v1Eid && value > 0) revert Executor_UnsupportedOptionType(optionType);

                dstAmount += value;
                lzReceiveGas += gas;
            } else if (optionType == ExecutorOptions.OPTION_TYPE_NATIVE_DROP) {
                (uint128 nativeDropAmount, ) = ExecutorOptions.decodeNativeDropOption(option);
                dstAmount += nativeDropAmount;
            } else if (optionType == ExecutorOptions.OPTION_TYPE_LZCOMPOSE) {
                // endpoint v1 does not support lzCompose
                if (_v1Eid) revert Executor_UnsupportedOptionType(optionType);

                (, uint128 gas, uint128 value) = ExecutorOptions.decodeLzComposeOption(option);
                dstAmount += value;
                totalGas += gas;
            } else if (optionType == ExecutorOptions.OPTION_TYPE_ORDERED_EXECUTION) {
                ordered = true;
            } else {
                revert Executor_UnsupportedOptionType(optionType);
            }
        }
        if (cursor != _options.length) revert Executor_InvalidExecutorOptions(cursor);
        if (dstAmount > _nativeCap) revert Executor_NativeAmountExceedsCap(dstAmount, _nativeCap);
        if (lzReceiveGas == 0) revert Executor_ZeroLzReceiveGasProvided();
        totalGas += lzReceiveGas;

        if (ordered) {
            totalGas = (totalGas * 102) / 100;
        }
    }

    function _applyPremiumToGas(
        uint256 _fee,
        uint16 _bps,
        uint16 _defaultBps,
        uint128 _marginUSD,
        uint128 _nativePriceUSD
    ) internal view returns (uint256) {
        uint16 multiplierBps = _bps == 0 ? _defaultBps : _bps;

        uint256 feeWithMultiplier = (_fee * multiplierBps) / 10000;

        if (_nativePriceUSD == 0 || _marginUSD == 0) {
            return feeWithMultiplier;
        }
        uint256 feeWithMargin = (_marginUSD * nativeDecimalsRate) / _nativePriceUSD + _fee;
        return feeWithMargin > feeWithMultiplier ? feeWithMargin : feeWithMultiplier;
    }

You can see the Price Feed from L0 which implements dynamic pricing based on chain average gas fees, which are relayed by a trusted entity

https://github.com/LayerZero-Labs/LayerZero-v2/blob/142846c3d6d51e3c2a0852c41b4c2b63fcda5a0a/messagelib/contracts/PriceFeed.sol#L152-L164

    function estimateFeeByChain(
        uint16 _dstEid,
        uint256 _callDataSize,
        uint256 _gas
    ) external view returns (uint256 fee, uint128 priceRatio) {
        if (_dstEid == 110 || _dstEid == 10143 || _dstEid == 20143) {
            return _estimateFeeWithArbitrumModel(_dstEid, _callDataSize, _gas);
        } else if (_dstEid == 111 || _dstEid == 10132 || _dstEid == 20132) {
            return _estimateFeeWithOptimismModel(_dstEid, _callDataSize, _gas);
        } else {
            return _estimateFeeWithDefaultModel(_dstEid, _callDataSize, _gas);
        }
    }

The L0 library will be pricing costs dynamically, meaning that a hardcoded fee can cause reverts when gas prices raise sufficiently fast

Mitigation

Use fee.nativeFee to enforce a minimum, but allow passing msg.value to avoid this edge case

Assessed type

MEV

c4-judge commented 5 months ago

dmvt marked the issue as primary issue

c4-sponsor commented 5 months ago

0xRektora (sponsor) confirmed

c4-judge commented 5 months ago

dmvt marked the issue as satisfactory

c4-judge commented 5 months ago

dmvt marked the issue as selected for report

c4-sponsor commented 5 months ago

0xRektora (sponsor) disputed

c4-sponsor commented 5 months ago

0xRektora (sponsor) confirmed

c4-sponsor commented 5 months ago

0xRektora marked the issue as disagree with severity

0xRektora commented 5 months ago

@dmvt It should be a QA. The fee is computed off-chain, and will account for any price changes. Even if we implemented the proposed mitigation, at the end it's at the source chain that the necessary amount needs to be computed.

dmvt commented 5 months ago

Agreed

c4-judge commented 5 months ago

dmvt changed the severity to QA (Quality Assurance)

c4-judge commented 5 months ago

dmvt marked the issue as grade-a

c4-judge commented 5 months ago

dmvt marked the issue as not selected for report