Fix Incorrect handling of requesterCallData for Flashloan action inside _getBeneficiaryFromCalldataof the router

[pedrovalido]

This PR addresses H-7

[pedrovalido]

@DaigaroCota please provide an idea of how to test. What we need is something similar to the test_bridgeOutbound with some kind of check for the 2nd action and the 1st one being the flashloan.

[0xdcota]

@pedrovalido I did not finish but it is something along this lines: Please ask questions:

function test_simpleFlashloan() public {
    uint256 amount = 2 ether;

    MockTestFlasher flasher = new MockTestFlasher();
    deal(collateralAsset, ALICE, amount);

    do_deposit(amount, vault, ALICE);

    vm.startPrank(ALICE);
    SafeERC20.safeApprove(IERC20(address(vault)), address(connextRouter), amount);
    vm.stopPrank();

    // The maximum slippage acceptable, in BPS, due to the Connext bridging mechanics
    // Eg. 0.05% slippage threshold will be 5.
    uint256 slippageThreshold = 0;

    uint32 destDomain = OPTIMISM_GOERLI_DOMAIN;

    IRouter.Action[] memory originActions = new IRouter.Action[](2);
    bytes[] memory originArgs = new bytes[](2);
    originActions[0] = IRouter.Action.Withdraw;
    originArgs[0] = abi.encode(address(vault), amount / 2, address(connextRouter), ALICE);
    originActions[1] = IRouter.Action.XTransferWithCall;

    IRouter.Action[] memory destActions = new IRouter.Action[](1);
    bytes[] memory destArgs = new bytes[](1);
    destActions[0] = IRouter.Action.Flashloan;

    // Perform a flashclose
    IRouter.Action[] memory destInnerActions = new IRouter.Action[](4);
    bytes[] memory destInnerArgs = new bytes[](4);
    destInnerActions[0] = IRouter.Action.Payback;
    destInnerArgs[0] = abi.encode(address(vault), amount, ALICE, address(connextRouter));
    destInnerActions[1] = IRouter.Action.PermitWithdraw;
    destInnerActions[2] = IRouter.Action.Withdraw;
    destInnerActions[3] = IRouter.Action.Swap;

    bytes memory requestorCalldata =
      abi.encodeWithSelector(IRouter.xBundle.selector, destInnerActions, destInnerArgs);
    destArgs[0] = abi.encode(
      address(flasher), collateralAsset, amount, address(connextRouter), requestorCalldata
    );
    bytes memory destCallData = abi.encode(destActions, destArgs, slippageThreshold);
    originArgs[1] =
      abi.encode(destDomain, slippageThreshold, collateralAsset, amount / 2, destCallData);

    vm.expectEmit(false, false, false, false);
    emit Dispatch("", 1, "", "");
    connextRouter.xBundle(originActions, originArgs);
  }

[0xdcota]

The destination actions in fact do not matter.. in the test I am pretending to encode a flashloan-that "intends" to do a flashclose. However, it is providing data to test that the gettting of the beneficiary from calldata works.

[0xdcota]

I merged the beneficiary changes of PR #389 because there were changes that were best to account in here involving beneficiary checks.

[pedrovalido]

I merged the beneficiary changes of PR #389 because there were changes that were best to account in here involving beneficiary checks.

Great :)

[0xcuriousapple]

LGTM Did not review LibBytes library since its out of scope

Some comments : The slicing should work, However, I think abi.decode(requestorCalldata, (BaseRouter.xBundle.selector, Actions[], bytes[])); would have been much clear.

[0xdcota]

LGTM Did not review LibBytes library since its out of scope

Some comments : The slicing should work, However, I think abi.decode(requestorCalldata, (BaseRouter.xBundle.selector, Actions[], bytes[])); would have been much clear.

We imported the library LibBytes from of a member of the Consensys team, and we added no modifications. We feel comfortable using it.

Your proposal seems elegant, but I think it does not compile:

Error:
Compiler run failed:
Error (1039): Argument has to be a type name.
   --> src/routers/ConnextRouter.sol:398:40:
    |
398 |         abi.decode(requestorCalldata, (BaseRouter.xBundle.selector, Action[], bytes[]));
    |                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^

Error (9574): Type tuple() is not implicitly convertible to expected type enum IRouter.Action[] memory.
   --> src/routers/ConnextRouter.sol:397:7:
    |
397 |       (Action[] memory newActions, bytes[] memory newArgs,) =
    |       ^ (Relevant source part starts here and spans across multiple lines).

Error (9574): Type enum IRouter.Action[] memory is not implicitly convertible to expected type bytes memory[] memory.
   --> src/routers/ConnextRouter.sol:397:7:
    |
397 |       (Action[] memory newActions, bytes[] memory newArgs,) =
    |       ^ (Relevant source part starts here and spans across multiple lines).

[0xcuriousapple]

hmmm, there is no decode with selector, my bad it has to be bytes4 instead, I think

 (, Action[] memory newActions, bytes[] memory newArgs) = abi.decode(
      requestorCalldata, (bytes4, Action[], bytes[]));

[0xcuriousapple]

yes should compile now

[0xdcota]

yes should compile now

I think we tried, It does compile now but it reverts during execution: I actually had to test this on a different branch, in where the test_simpleFlashloan was updated to actually test checking the beneficiary. For reference: protocol/fix/sender-param-crosstranfer-with-call

If I recall well now, the only way to make it work, was to chop-off those 4 bytes from the data.