Skip to content
Merged
117 changes: 68 additions & 49 deletions contracts/SpokePoolPeriphery.sol
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,10 @@ contract SpokePoolPeriphery is SpokePoolPeripheryInterface, ReentrancyGuard, Mul
// Mapping from user address to their current nonce
mapping(address => uint256) public permitNonces;

// Witness identifiers for the bridge and swap functions. Used to ensure collisions can't happen.
bytes32 public constant BRIDGE_AND_SWAP_WITNESS_IDENTIFIER = keccak256("BridgeAndSwapWitness");
bytes32 public constant BRIDGE_WITNESS_IDENTIFIER = keccak256("BridgeWitness");

event SwapBeforeBridge(
address exchange,
bytes exchangeCalldata,
Expand Down Expand Up @@ -286,7 +290,10 @@ contract SpokePoolPeriphery is SpokePoolPeripheryInterface, ReentrancyGuard, Mul
PeripherySigningLib.hashSwapAndDepositData(swapAndDepositData),
swapAndDepositDataSignature
);
_swapAndBridge(swapAndDepositData);
// Copy struct to memory and set nonce to originalNonce + 1 to avoid 0 (which triggers regular deposit)
SwapAndDepositData memory modifiedData = swapAndDepositData;
modifiedData.nonce = swapAndDepositData.nonce + 1;
_swapAndBridge(modifiedData);
}

/**
Expand Down Expand Up @@ -341,9 +348,12 @@ contract SpokePoolPeriphery is SpokePoolPeripheryInterface, ReentrancyGuard, Mul
SwapAndDepositData calldata swapAndDepositData,
uint256 validAfter,
uint256 validBefore,
bytes calldata receiveWithAuthSignature,
bytes calldata swapAndDepositDataSignature
bytes calldata receiveWithAuthSignature
) external override nonReentrant {
bytes32 witness = keccak256(
abi.encodePacked(BRIDGE_AND_SWAP_WITNESS_IDENTIFIER, abi.encode(swapAndDepositData))
);
Copy link
Contributor

Choose a reason for hiding this comment

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

how do we ensure witness uniqueness given that its now obtained from deposit data?

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that the token implementing ERC-3009 will revert on the receiveWithAuthorization call if the witness has already been used

Also there's a nonce in SwapAndDepositData thats part of what gets hashed to generate the witness - so this can create uniqueness even if all the other intent data is identical

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, my undertsanding was the same as Taylor's, witness here is essentially the nonce for the ERC-3009 token's permit with auth

From https://eips.ethereum.org/EIPS/eip-3009 reference implementation:

        require(
            !_authorizationStates[from][nonce],
            "EIP3009: authorization is used"
        );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed with the above. There is nonce reuse prevention that is per-user. The nonce within the hashed data means that a user should never have an identical witness hash unless they are accidentally reusing the same API response twice.


(bytes32 r, bytes32 s, uint8 v) = PeripherySigningLib.deserializeSignature(receiveWithAuthSignature);
uint256 _submissionFeeAmount = swapAndDepositData.submissionFees.amount;
// While any contract can vacuously implement `receiveWithAuthorization` (or just have a fallback),
Expand All @@ -355,7 +365,7 @@ contract SpokePoolPeriphery is SpokePoolPeripheryInterface, ReentrancyGuard, Mul
swapAndDepositData.swapTokenAmount + _submissionFeeAmount,
validAfter,
validBefore,
bytes32(swapAndDepositData.nonce),
witness,
v,
r,
s
Expand All @@ -368,20 +378,10 @@ contract SpokePoolPeriphery is SpokePoolPeripheryInterface, ReentrancyGuard, Mul

// Note: No need to validate our internal nonce for receiveWithAuthorization
// as EIP-3009 has its own nonce mechanism that prevents replay attacks.
//
// Design Decision: We reuse the receiveWithAuthorization nonce for our signatures,
// but not for permit, which creates a theoretical replay attack that we think is
// incredibly unlikely because this would require:
// 1. A token implementing both ERC-2612 and ERC-3009
// 2. A user using the same nonces for swapAndBridgeWithPermit and for swapAndBridgeWithAuthorization
// 3. Issuing these signatures within a short amount of time (limited by fillDeadlineBuffer)
// Verify that the signatureOwner signed the input swapAndDepositData.
_validateSignature(
signatureOwner,
PeripherySigningLib.hashSwapAndDepositData(swapAndDepositData),
swapAndDepositDataSignature
);
_swapAndBridge(swapAndDepositData);
// We use the witness (which serves as the ERC-3009 nonce) as the deposit nonce.
SwapAndDepositData memory modifiedData = swapAndDepositData;
modifiedData.nonce = uint256(witness);
_swapAndBridge(modifiedData);
}

/**
Expand Down Expand Up @@ -413,6 +413,7 @@ contract SpokePoolPeriphery is SpokePoolPeripheryInterface, ReentrancyGuard, Mul
_validateAndIncrementNonce(signatureOwner, depositData.nonce);
// Verify that the signatureOwner signed the input depositData.
_validateSignature(signatureOwner, PeripherySigningLib.hashDepositData(depositData), depositDataSignature);
// Use nonce + 1 to avoid 0 (which triggers regular deposit) and ensure uniqueness
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment is no longer accurate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

_deposit(
depositData.spokePool,
depositData.baseDepositData.depositor,
Expand All @@ -423,6 +424,7 @@ contract SpokePoolPeriphery is SpokePoolPeripheryInterface, ReentrancyGuard, Mul
depositData.baseDepositData.outputAmount,
depositData.baseDepositData.destinationChainId,
depositData.baseDepositData.exclusiveRelayer,
depositData.nonce + 1, // +1 to avoid 0 (which triggers regular deposit) and ensure uniqueness
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why this is needed. If the relayer is doing a pre-fill and a deposit after, wouldn't they just submit a non-zero nonce? Feels like it should be their responsibility

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, I take it back, the user signed over depositData. Wouldn't we, then, trigger a lot of deposits to have nonce = 1?

I like the solution proposed for the Permit2: hashing this with the permit nonce seems like it'd ensure uniqueness

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, why are we forcing the user to have a non-zero nonce here, but not in Permit2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point -- let me see if we can allow 0 nonces on all of them.

depositData.baseDepositData.quoteTimestamp,
depositData.baseDepositData.fillDeadline,
depositData.baseDepositData.exclusivityParameter,
Expand Down Expand Up @@ -460,6 +462,7 @@ contract SpokePoolPeriphery is SpokePoolPeripheryInterface, ReentrancyGuard, Mul
_submissionFeeAmount
);

// User controls the nonce in permit2 flows - if 0, uses regular deposit; if non-zero, uses unsafe deposit
_deposit(
depositData.spokePool,
depositData.baseDepositData.depositor,
Expand All @@ -470,6 +473,7 @@ contract SpokePoolPeriphery is SpokePoolPeripheryInterface, ReentrancyGuard, Mul
depositData.baseDepositData.outputAmount,
depositData.baseDepositData.destinationChainId,
depositData.baseDepositData.exclusiveRelayer,
depositData.nonce, // TODO: should we hash this nonce (if nonzero) with the permit2 nonce to protect the user against nonce reuse?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, hashing it together sounds right to me. Then the relayer would know to pre-fill with the hash that they can derive easily right

depositData.baseDepositData.quoteTimestamp,
depositData.baseDepositData.fillDeadline,
depositData.baseDepositData.exclusivityParameter,
Expand All @@ -485,9 +489,9 @@ contract SpokePoolPeriphery is SpokePoolPeripheryInterface, ReentrancyGuard, Mul
DepositData calldata depositData,
uint256 validAfter,
uint256 validBefore,
bytes calldata receiveWithAuthSignature,
bytes calldata depositDataSignature
bytes calldata receiveWithAuthSignature
) external override nonReentrant {
bytes32 witness = keccak256(abi.encodePacked(BRIDGE_WITNESS_IDENTIFIER, abi.encode(depositData)));
// Load variables used multiple times onto the stack.
uint256 _inputAmount = depositData.inputAmount;
uint256 _submissionFeeAmount = depositData.submissionFees.amount;
Expand All @@ -500,7 +504,7 @@ contract SpokePoolPeriphery is SpokePoolPeripheryInterface, ReentrancyGuard, Mul
_inputAmount + _submissionFeeAmount,
validAfter,
validBefore,
bytes32(depositData.nonce),
witness,
v,
r,
s
Expand All @@ -513,15 +517,7 @@ contract SpokePoolPeriphery is SpokePoolPeripheryInterface, ReentrancyGuard, Mul

// Note: No need to validate our internal nonce for receiveWithAuthorization
// as EIP-3009 has its own nonce mechanism that prevents replay attacks.
//
// Design Decision: We reuse the receiveWithAuthorization nonce for our signatures,
// but not for permit, which creates a theoretical replay attack that we think is
// incredibly unlikely because this would require:
// 1. A token implementing both ERC-2612 and ERC-3009
// 2. A user using the same nonces for depositWithPermit and for depositWithAuthorization
// 3. Issuing these signatures within a short amount of time (limited by fillDeadlineBuffer)
// Verify that the signatureOwner signed the input depositData.
_validateSignature(signatureOwner, PeripherySigningLib.hashDepositData(depositData), depositDataSignature);
// We use the witness (which serves as the ERC-3009 nonce) as the deposit nonce.
_deposit(
depositData.spokePool,
depositData.baseDepositData.depositor,
Expand All @@ -532,6 +528,7 @@ contract SpokePoolPeriphery is SpokePoolPeripheryInterface, ReentrancyGuard, Mul
depositData.baseDepositData.outputAmount,
depositData.baseDepositData.destinationChainId,
depositData.baseDepositData.exclusiveRelayer,
uint256(witness),
depositData.baseDepositData.quoteTimestamp,
depositData.baseDepositData.fillDeadline,
depositData.baseDepositData.exclusivityParameter,
Expand Down Expand Up @@ -571,16 +568,18 @@ contract SpokePoolPeriphery is SpokePoolPeripheryInterface, ReentrancyGuard, Mul
}

/**
* @notice Approves the spoke pool and calls `depositV3` function with the specified input parameters.
* @param depositor The address on the origin chain which should be treated as the depositor by Across, and will therefore receive refunds if this deposit
* is unfilled.
* @notice Approves the spoke pool and calls either `depositV3` or `unsafeDeposit` based on whether a nonce is provided.
* @dev When depositNonce is 0, calls the regular deposit function. When non-zero, calls the unsafe deposit variant.
* @param spokePool The address of the spoke pool to deposit into.
* @param depositor The address on the origin chain which should be treated as the depositor by Across.
* @param recipient The address on the destination chain which should receive outputAmount of outputToken.
* @param inputToken The token to deposit on the origin chain.
* @param outputToken The token to receive on the destination chain.
* @param inputAmount The amount of the input token to deposit.
* @param outputAmount The amount of the output token to receive.
* @param destinationChainId The network ID for the destination chain.
* @param exclusiveRelayer The optional address for an Across relayer which may fill the deposit exclusively.
* @param depositNonce The nonce for this deposit. If 0, calls regular deposit; if non-zero, calls unsafe deposit.
* @param quoteTimestamp The timestamp at which the relay and LP fee was calculated.
* @param fillDeadline The timestamp at which the deposit must be filled before it will be refunded by Across.
* @param exclusivityParameter The deadline or offset during which the exclusive relayer has rights to fill the deposit without contention.
Expand All @@ -596,33 +595,52 @@ contract SpokePoolPeriphery is SpokePoolPeripheryInterface, ReentrancyGuard, Mul
uint256 outputAmount,
uint256 destinationChainId,
bytes32 exclusiveRelayer,
uint256 depositNonce,
uint32 quoteTimestamp,
uint32 fillDeadline,
uint32 exclusivityParameter,
bytes calldata message
bytes memory message
) private {
IERC20(inputToken).forceApprove(spokePool, inputAmount);
V3SpokePoolInterface(spokePool).deposit(
depositor.toBytes32(),
recipient,
inputToken.toBytes32(),
outputToken,
inputAmount,
outputAmount,
destinationChainId,
exclusiveRelayer,
quoteTimestamp,
fillDeadline,
exclusivityParameter,
message
);
if (depositNonce == 0) {
V3SpokePoolInterface(spokePool).deposit(
depositor.toBytes32(),
recipient,
inputToken.toBytes32(),
outputToken,
inputAmount,
outputAmount,
destinationChainId,
exclusiveRelayer,
quoteTimestamp,
fillDeadline,
exclusivityParameter,
message
);
} else {
V3SpokePoolInterface(spokePool).unsafeDeposit(
depositor.toBytes32(),
recipient,
inputToken.toBytes32(),
outputToken,
inputAmount,
outputAmount,
destinationChainId,
exclusiveRelayer,
depositNonce,
quoteTimestamp,
fillDeadline,
exclusivityParameter,
message
);
}
}

/**
* @notice Swaps a token on the origin chain before depositing into the Across spoke pool atomically.
* @param swapAndDepositData The parameters to use when calling both the swap on an exchange and bridging via an Across spoke pool.
*/
function _swapAndBridge(SwapAndDepositData calldata swapAndDepositData) private {
function _swapAndBridge(SwapAndDepositData memory swapAndDepositData) private {
// Load variables we use multiple times onto the stack.
IERC20 _swapToken = IERC20(swapAndDepositData.swapToken);
IERC20 _acrossInputToken = IERC20(swapAndDepositData.depositData.inputToken);
Expand Down Expand Up @@ -681,6 +699,7 @@ contract SpokePoolPeriphery is SpokePoolPeripheryInterface, ReentrancyGuard, Mul
adjustedOutputAmount,
swapAndDepositData.depositData.destinationChainId,
swapAndDepositData.depositData.exclusiveRelayer,
swapAndDepositData.nonce,
swapAndDepositData.depositData.quoteTimestamp,
swapAndDepositData.depositData.fillDeadline,
swapAndDepositData.depositData.exclusivityParameter,
Expand Down
8 changes: 2 additions & 6 deletions contracts/interfaces/SpokePoolPeripheryInterface.sol
Original file line number Diff line number Diff line change
Expand Up @@ -203,15 +203,13 @@ interface SpokePoolPeripheryInterface {
* @param validAfter The unix time after which the `receiveWithAuthorization` signature is valid.
* @param validBefore The unix time before which the `receiveWithAuthorization` signature is valid.
* @param receiveWithAuthSignature EIP3009 signature encoded as (bytes32 r, bytes32 s, uint8 v).
* @param swapAndDepositDataSignature The signature against the input swapAndDepositData encoded as (bytes32 r, bytes32 s, uint8 v).
*/
function swapAndBridgeWithAuthorization(
address signatureOwner,
SwapAndDepositData calldata swapAndDepositData,
uint256 validAfter,
uint256 validBefore,
bytes calldata receiveWithAuthSignature,
bytes calldata swapAndDepositDataSignature
bytes calldata receiveWithAuthSignature
) external;

/**
Expand Down Expand Up @@ -265,15 +263,13 @@ interface SpokePoolPeripheryInterface {
* @param validAfter The unix time after which the `receiveWithAuthorization` signature is valid.
* @param validBefore The unix time before which the `receiveWithAuthorization` signature is valid.
* @param receiveWithAuthSignature EIP3009 signature encoded as (bytes32 r, bytes32 s, uint8 v).
* @param depositDataSignature The signature against the input depositData encoded as (bytes32 r, bytes32 s, uint8 v).
*/
function depositWithAuthorization(
address signatureOwner,
DepositData calldata depositData,
uint256 validAfter,
uint256 validBefore,
bytes calldata receiveWithAuthSignature,
bytes calldata depositDataSignature
bytes calldata receiveWithAuthSignature
) external;

/**
Expand Down
3 changes: 3 additions & 0 deletions foundry.lock
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,8 @@
"name": "v1.11.0",
"rev": "8e40513d678f392f398620b3ef2b418648b33e89"
}
},
"lib/sp1-contracts": {
"rev": "512b5e029abc27f6e46a3c7eba220dac83ecc306"
}
}
Loading