sujithsomraaj / lifi-stargate-v2-audit

3 Day Review [10 Jun 2024 - 13 Jun 2024]
0 stars 0 forks source link

Ensure `oftCmd` length is always zero for bridge with dst swaps #14

Open sujithsomraaj opened 5 months ago

sujithsomraaj commented 5 months ago

Context: StargateFacetV2.sol#L144

Description: The stargate SendParam used in startBridgeTokensViaStargate and swapAndStartBridgeTokensViaStargate will have an optional parameter called oftCmd that determines whether to use taxi or bus in Stargate.

The issue here is that if the length of oftCmd is not zero, then no message will be composed at the destination for dst swaps, resulting in unexpected behavior. As funds could be recovered from the ReceiverStargateV2 contract, considering this a low severity issue.

Recommendation: Validate if the _bridgeData.hasDestinationCall is enabled then the oftCmd length should be zero.

function _validateDestinationCallFlag(
    ILiFi.BridgeData memory _bridgeData,
    StargateData calldata _stargateData
) private pure {
    if (
        _stargateData.sendParams.composeMsg.length > 0  !=
        _bridgeData.hasDestinationCall 
    ) {
        require(_stargateData.sendParams.oftCmd.length == 0); --> add this validation
        revert InformationMismatch();
    }
}

LI.FI:

Researcher:

0xDEnYO commented 5 months ago

Hi @sujithsomraaj ,

good point. I would add it but prefer to have all conditions in the if clause (so that the same error InformationMismatch is thrown). Like this:

    function _validateDestinationCallFlag(
        ILiFi.BridgeData memory _bridgeData,
        StargateData calldata _stargateData
    ) private pure {
      if (
          (_stargateData.sendParams.composeMsg.length > 0 != _bridgeData.hasDestinationCall) ||
          (_bridgeData.hasDestinationCall && _stargateData.sendParams.oftCmd.length != 0)
      ) {
          revert InformationMismatch();
      }
    }

Should be the same logic. Could you please confirm?

sujithsomraaj commented 5 months ago

yea they look good to me @0xDEnYO

0xDEnYO commented 5 months ago

New solution accepted - issue closed.