Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overriding transfer handler #419

23 changes: 21 additions & 2 deletions packages/protocol/src/routers/ConnextHandler.sol
Expand Up @@ -50,6 +50,9 @@ contract ConnextHandler {
/// @dev Custom errors
error ConnextHandler__callerNotConnextRouter();

bytes32 private constant ZERO_BYTES32 =
0x0000000000000000000000000000000000000000000000000000000000000000;

ConnextRouter public immutable connextRouter;

/// @dev Maps a failed transferId -> calldata
Expand Down Expand Up @@ -86,6 +89,20 @@ contract ConnextHandler {
return _failedTxns[transferId];
}

/**
* @notice Returns the true if the failed transaction is already recorded.
*
* @param transferId the unique identifier of the cross-chain txn
*/
function isTransferIdRecorded(bytes32 transferId) public view returns (bool) {
FailedTxn memory ftxn = _failedTxns[transferId];
if (ftxn.transferId != ZERO_BYTES32 && ftxn.originDomain != 0) {
return true;
} else {
return false;
}
}

/**
* @notice Records a failed {ConnextRouter-xReceive} call.
*
Expand Down Expand Up @@ -114,8 +131,10 @@ contract ConnextHandler {
external
onlyConnextRouter
{
_failedTxns[transferId] =
FailedTxn(transferId, amount, asset, originSender, originDomain, actions, args);
if (!isTransferIdRecorded(transferId)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this solves overriding vector, but adds a different attacking vector
now anyone can pre-call xBundle with the given transferId and make the legit recordFail revert

we had mentioned this previously as well
https://www.notion.so/0xmacro/Leads-Questions-on-Router-14d2593767a640258c902031573da7c8?pvs=4#c508aacbca3847aca17a1372879ed074
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @abhishekvispute good point, decided to essentially record all attempted transferIds with an increasing nonce.
Please refer to #536.

_failedTxns[transferId] =
FailedTxn(transferId, amount, asset, originSender, originDomain, actions, args);
}
}

/**
Expand Down
67 changes: 67 additions & 0 deletions packages/protocol/test/forking/goerli/ConnextRouter.t.sol
Expand Up @@ -446,6 +446,73 @@ contract ConnextRouterForkingTest is Routines, ForkingSetup {
assertEq(vault.balanceOfDebt(ALICE), borrowAmount);
}

function test_overridingFailedTransferInHandler() public {
bytes32 transferId_ = 0x000000000000000000000000000000000000000000000000000000000000000a;
uint256 amount = 2 ether;
uint256 borrowAmount = 1000e6;

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

// This calldata has to fail and funds handled accordingly by the router.
bytes memory failingCallData = _getDepositAndBorrowCallData(
ALICE, ALICE_PK, amount, borrowAmount, address(0), address(vault), slippageThreshold
);

// send directly the bridged funds to our router
// thus mocking Connext behavior
deal(collateralAsset, address(connextRouter), amount);

vm.startPrank(registry[domain].connext);
// call from OPTIMISM_GOERLI where 'originSender' is router that's supposed to have
// the same address as the one on GOERLI
connextRouter.xReceive(
transferId_,
amount,
vault.asset(),
address(connextRouter),
OPTIMISM_GOERLI_DOMAIN,
failingCallData
);
vm.stopPrank();

// Assert handler has recorded failed transfer
ConnextHandler.FailedTxn memory ftxn = connextHandler.getFailedTransaction(transferId_);
assertEq(ftxn.transferId, transferId_);
assertEq(ftxn.asset, collateralAsset);
assertEq(ftxn.amount, amount);

// Create different calldata
uint256 newAmount = 1.5 ether;
bytes memory newfailingCallData = _getDepositAndBorrowCallData(
ALICE, ALICE_PK, newAmount, borrowAmount, address(0), address(vault), slippageThreshold
);

// send directly the bridged funds to our router
// thus mocking Connext behavior
deal(collateralAsset, address(connextRouter), newAmount);

vm.startPrank(registry[domain].connext);
// call from OPTIMISM_GOERLI where 'originSender' is router that's supposed to have
// the same address as the one on GOERLI
connextRouter.xReceive(
transferId_,
newAmount,
vault.asset(),
address(connextRouter),
OPTIMISM_GOERLI_DOMAIN,
newfailingCallData
);
vm.stopPrank();

// Assert handler has not modified the recorded failed transfer
ConnextHandler.FailedTxn memory newftxn = connextHandler.getFailedTransaction(transferId_);
assertEq(newftxn.transferId, transferId_);
assertEq(newftxn.asset, collateralAsset);
assertEq(newftxn.amount, amount);
}

function test_simpleFlashloan() public {
uint256 amount = 2 ether;
MockTestFlasher flasher = new MockTestFlasher();
Expand Down