Jeiwan - `XProvider` forces increased relayer fees when transferring tokens cross-chain

[sherlock-admin]

Jeiwan

medium

XProvider forces increased relayer fees when transferring tokens cross-chain

Summary

When transferring tokens cross-chain, XProvider sends two cross-chain message, while only one can be sent. Whoever triggers cross-chain token transfers (which are required to complete rebalancing) will pay relayer fees twice.

Vulnerability Detail

The protocol integrates with Connext to handle cross-chain interactions. XProvider is a contract that manages interactions between vaults deployed on all supported networks and XChainController. XProvider is deployed on each of the network where a vault is deployed and is used to send and receive cross-chain messages via Connext. Among other things, XProvider handles cross-chain token transfers during vaults rebalancing:

1. xTransferToController transfers funds from a vault to the XChainController;
2. xTransferToVaults transfers funds from the XChainController to a vault.

The two functions, besides sending tokens, also update the state in the destination contract:

1. xTransferToController calls XChainController.upFundsReceived to update the counter of vaults that have sent tokens to XChainController;
2. xTransferToVaults calls Vault.receiveFunds to set a "funds received" flag in the vault.

Both sending tokens and triggering a state change send a cross-chain message by calling IConnext.xcall:

1. xSend calls IConnext.xcall and sends relayer fee along the call;
2. xTransfer sends tokens by calling IConnext.xcall, and it also requires paying relayer fee.

Thus, the caller of xTransferToController and xTransferToVaults will have to pay double relayer fee. Since these functions are mandatory for rebalancing, the extra fee will have to be paid by the guardian or any actor who manages vaults rebalancing. However, Connext allows to transfer tokens and make arbitrary calls in one message, while paying relayer fee only once.

Impact

xTransferToController and xTransferToVaults incur double relayer fees on the caller. The extra cost will have to be paid by whoever manages rebalancing.

Code Snippet

1. xTransferToController calls xTransfer and pushFeedbackToXController–both of them create a cross-chain message:
   https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/XProvider.sol#L321
2. xTransferToVaults calls xTransfer and pushFeedbackToVault–both of them create a cross-chain message:
   https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/XProvider.sol#L362

Tool used

Manual Review

Recommendation

According to the documentation of Connext, the _callData argument of xcall doesn't need to be empty when transferring tokens: xReceive can handle both of them together:

function xReceive(
    bytes32 _transferId,
    uint256 _amount,
    address _asset,
    address _originSender,
    uint32 _origin,
    bytes memory _callData
) external returns (bytes memory) {
    // Check for the right token
    require(
      _asset == address(token),
      "Wrong asset received"
    );

    // Enforce a cost to update the greeting
    require(
      _amount > 0,
      "Must pay at least 1 wei"
    );

    // Unpack the _callData
    string memory newGreeting = abi.decode(_callData, (string));
    _updateGreeting(newGreeting);
}

Also, judging by the implementation of the Connext contract, the passed calldata is executed even when tokens are transferred:

// transfer funds to recipient
AssetLogic.handleOutgoingAsset(_asset, _args.params.to, _amountOut);

// execute the calldata
_executeCalldata(_transferId, _amountOut, _asset, _reconciled, _args.params);

Thus, in xTransferToController and xTransferToVaults, consider passing the calldata of the second calls to xTransfer.

[mister0y]

No funds are at risk here. We don't know if it's smart to fix this. There is a good argument to be made to keep xchain actions as simple as possible, therefore separating the transfer of funds and the messaging.

[Theezr]

Fix:
https://github.com/derbyfinance/derby-yield-optimiser/pull/199