Jeiwan - Cross-chain message authentication can be bypassed, allowing an attacker to disrupt the state of vaults

[sherlock-admin]

Jeiwan

high

Cross-chain message authentication can be bypassed, allowing an attacker to disrupt the state of vaults

Summary

A malicious actor may send a cross-chain message to an XProvider contract and bypass the onlySource authentication check. As a result, they'll be able to call any function in the XProvider contract that has the onlySource modifier and disrupt the state of XChainController and all vaults.

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. XProvider is a core contract that handles vault rebalancing, transferring of allocations from Game to XChainController and to vaults, transferring of tokens deposited to vaults between vault on different networks. Thus, it's critical that the functions of this contract are only called by authorized actors.

To ensure that cross-chain messages are sent from authorized actors, there's onlySource modifier that's applied to the xReceive function. The modifier checks that the sender of a message is trusted:

modifier onlySource(address _originSender, uint32 _origin) {
  require(_originSender == trustedRemoteConnext[_origin] && msg.sender == connext, "Not trusted");
  _;
}

However, it doesn't check that trustedRemoteConnext[_origin] is set (i.e. it's not the zero address), and _originSender can in fact be the zero address.

In Connext, a message can be delivered via one of the two paths: the fast path or the slow path. The fast path is taken when, on the destination, message receiving is not authentication, i.e. when destination allows receiving of messages from all senders. The slow path is taken when message receiving on the destination is authenticated, i.e. destination allows any sender (it doesn't check a sender).

Since, XProvider always checks the sender of a message, only the slow path will be used by Connext to deliver messages to it. However, Connext always tries the slow path:

Routers observing the origin chain with funds on the destination chain will:
Simulate the transaction (if this fails, the assumption is that this is a more "expressive" crosschain message that requires authentication and so must go through the AMB: the slow path).

I.e. it'll always send a message and see if it reverts on the destination or not: if it does, Connext will switch to the slow path.

When Connext executes a message on the destination chain in the fast path, it sets the sender address to the zero address:

(bool success, bytes memory returnData) = ExcessivelySafeCall.excessivelySafeCall(
  _params.to,
  gasleft() - Constants.EXECUTE_CALLDATA_RESERVE_GAS,
  0, // native asset value (always 0)
  Constants.DEFAULT_COPY_BYTES, // only copy 256 bytes back as calldata
  abi.encodeWithSelector(
    IXReceiver.xReceive.selector,
    _transferId,
    _amount,
    _asset,
    _reconciled ? _params.originSender : address(0), // use passed in value iff authenticated
    _params.originDomain,
    _params.callData
  )
);

Thus, Connext will try to call the XProvider.xReceive function with the _originSender argument set to the zero address. And there are situations when the onlySource modifier will pass such calls: when the origin network (as specified by the _origin argument) is not in the trustedRemoteConnext mapping.

According to the description of the project, it'll be deployed on the following networks:

Mainnet, Arbitrum, Optimism, Polygon, Binance Smart Chain

And this is the list of networks supported by Connext:

Ethereum Mainnet
Polygon
Optimism
Arbitrum One
Gnosis Chain
BNB Chain

Thus, a malicious actor can send a message from Gnosis Chain (it's not supported by Derby), and the onlySource modifier will pass the message. The same is true for any new network supported by Connext in the future and not supported by Derby.

Impact

A malicious actor can call XProvider.xReceive and any functions of XProvider with the onlySelf modifier:

1. xReceive allow the caller to call any public function of XProvider, but only the ones with the onlySelf modifier are authorized;
2. receiveAllocations can be used to corrupt allocations in the XChainController (i.e. allocate all tokens only to the protocol the attacker will benefit the most from);
3. receiveTotalUnderlying can be used to set wrong "total underlying" value in the XChainController and block rebalancing of vaults (due to an underflow or another arithmetical error);
4. receiveSetXChainAllocation can be used to set an exchange rate that will allow an attacker to drain a vault by redeeming their LP tokens at a higher rate;
5. receiveFeedbackToXController can be used to trick the XChainController into skipping receiving of funds from a vault;
6. receiveProtocolAllocationsToVault can be used by an attacker to unilaterally set allocations in a vault, directing funds only to protocol the attacker will benefit from;
7. receiveRewardsToGame can be used by an attacker to increase the reward per LP token in a protocol the attacker deposited to;
8. finally, receiveStateFeedbackToVault can allow an attacker to switch off a vault and exclude it from rebalancing.

Code Snippet

1. onlySource modifier validates the message sender:
   https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/XProvider.sol#L85-L88
2. xReceive is protected by the onlySource modifier:
   https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/XProvider.sol#L170-L180
3. Connext always tries the fast path and sets the sender address to the zero address:
   https://github.com/connext/monorepo/blob/87b75b346664271522e2f2acfd10bebcfeb93993/packages/deployments/contracts/contracts/core/connext/facets/BridgeFacet.sol#L878

Tool used

Manual Review

Recommendation

In the onlySource modifier, consider checking that trustedRemoteConnext[_origin] doesn't return the zero address:

diff --git a/derby-yield-optimiser/contracts/XProvider.sol b/derby-yield-optimiser/contracts/XProvider.sol
index 6074fa0..f508a7c 100644
--- a/derby-yield-optimiser/contracts/XProvider.sol
+++ b/derby-yield-optimiser/contracts/XProvider.sol
@@ -83,7 +83,7 @@ contract XProvider is IXReceiver {
    *    3) The call to this contract comes from Connext.
    */
   modifier onlySource(address _originSender, uint32 _origin) {
-    require(_originSender == trustedRemoteConnext[_origin] && msg.sender == connext, "Not trusted");
+    require(trustedRemoteConnext[_origin] != address(0) && _originSender == trustedRemoteConnext[_origin] && msg.sender == connext, "Not trusted");
     _;
   }

[Jeiwan]

Escalate for 10 USDC

This was mistakenly marked as a duplicate.

This report points at the weak cross-chain messages authentication, which allows an attacker to send fake cross-chain messages and pass the authentication check. This basically disrupts the rebalancing and allows the attacker to manipulate token allocations for their profit (and for the loss of everyone else) or even lock rebalancing indefinitely.

[sherlock-admin]

Escalate for 10 USDC

This was mistakenly marked as a duplicate.

This report points at the weak cross-chain messages authentication, which allows an attacker to send fake cross-chain messages and pass the authentication check. This basically disrupts the rebalancing and allows the attacker to manipulate token allocations for their profit (and for the loss of everyone else) or even lock rebalancing indefinitely.

You've created a valid escalation for 10 USDC!

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.

[Theezr]

Valid high issue

[hrishibhat]

Escalation accepted

This is a valid high issue

[sherlock-admin]

Escalation accepted

This is a valid high issue

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[Theezr]

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