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.

Sign up for GitHub

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

feat(SpokePool): Introduce opt-in deterministic relay data hashes #583

Merged
merged 11 commits into from
Oct 8, 2024
Merged

feat(SpokePool): Introduce opt-in deterministic relay data hashes #583

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0688abd
feat(SpokePool): Introduce opt-in deterministic relay data hashes
nicholaspai Aug 20, 2024
28fcb30
change types
nicholaspai Aug 20, 2024
cbdb3b5
merge master
nicholaspai Sep 24, 2024
229f5ea
Update SpokePool.sol
nicholaspai Sep 24, 2024
9e732fb
Use different hashing algorithm, add tests
nicholaspai Sep 24, 2024
60589f1
Remove the keccak256
nicholaspai Sep 25, 2024
751ddf8
Add view function
nicholaspai Sep 25, 2024
19a3380
Hash packed deterministic deposit id
nicholaspai Sep 26, 2024
635c541
Update SpokePool.Deposit.ts
nicholaspai Sep 26, 2024
497a493
Include both depositor and msg.sender in the hash
nicholaspai Oct 1, 2024
78eb4c0
Update SpokePool.sol
nicholaspai Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
201 changes: 157 additions & 44 deletions contracts/SpokePool.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ abstract contract SpokePool is

bytes32 public constant UPDATE_V3_DEPOSIT_DETAILS_HASH =
keccak256(
"UpdateDepositDetails(uint32 depositId,uint256 originChainId,uint256 updatedOutputAmount,address updatedRecipient,bytes updatedMessage)"
"UpdateDepositDetails(uint256 depositId,uint256 originChainId,uint256 updatedOutputAmount,address updatedRecipient,bytes updatedMessage)"
);

// Default chain Id used to signify that no repayment is requested, for example when executing a slow fill.
Expand Down Expand Up @@ -548,59 +548,101 @@ abstract contract SpokePool is
uint32 exclusivityPeriod,
bytes calldata message
) public payable override nonReentrant unpausedDeposits {
// Check that deposit route is enabled for the input token. There are no checks required for the output token
// which is pulled from the relayer at fill time and passed through this contract atomically to the recipient.
if (!enabledDepositRoutes[inputToken][destinationChainId]) revert DisabledRoute();

// Require that quoteTimestamp has a maximum age so that depositors pay an LP fee based on recent HubPool usage.
// It is assumed that cross-chain timestamps are normally loosely in-sync, but clock drift can occur. If the
// SpokePool time stalls or lags significantly, it is still possible to make deposits by setting quoteTimestamp
// within the configured buffer. The owner should pause deposits/fills if this is undesirable.
// This will underflow if quoteTimestamp is more than depositQuoteTimeBuffer;
// this is safe but will throw an unintuitive error.

// slither-disable-next-line timestamp
uint256 currentTime = getCurrentTime();
if (currentTime - quoteTimestamp > depositQuoteTimeBuffer) revert InvalidQuoteTimestamp();

// fillDeadline is relative to the destination chain.
// Don’t allow fillDeadline to be more than several bundles into the future.
// This limits the maximum required lookback for dataworker and relayer instances.
// Also, don't allow fillDeadline to be in the past. This poses a potential UX issue if the destination
// chain time keeping and this chain's time keeping are out of sync but is not really a practical hurdle
// unless they are significantly out of sync or the depositor is setting very short fill deadlines. This latter
// situation won't be a problem for honest users.
if (fillDeadline < currentTime || fillDeadline > currentTime + fillDeadlineBuffer) revert InvalidFillDeadline();

// If the address of the origin token is a wrappedNativeToken contract and there is a msg.value with the
// transaction then the user is sending the native token. In this case, the native token should be
// wrapped.
if (inputToken == address(wrappedNativeToken) && msg.value > 0) {
if (msg.value != inputAmount) revert MsgValueDoesNotMatchInputAmount();
wrappedNativeToken.deposit{ value: msg.value }();
// Else, it is a normal ERC20. In this case pull the token from the caller as per normal.
// Note: this includes the case where the L2 caller has WETH (already wrapped ETH) and wants to bridge them.
// In this case the msg.value will be set to 0, indicating a "normal" ERC20 bridging action.
} else {
// msg.value should be 0 if input token isn't the wrapped native token.
if (msg.value != 0) revert MsgValueDoesNotMatchInputAmount();
IERC20Upgradeable(inputToken).safeTransferFrom(msg.sender, address(this), inputAmount);
}

emit V3FundsDeposited(
_depositV3(
depositor,
recipient,
inputToken,
outputToken,
inputAmount,
outputAmount,
destinationChainId,
exclusiveRelayer,
// Increment count of deposits so that deposit ID for this spoke pool is unique.
// @dev Implicitly casts from uint32 to uint256 by padding the left-most bytes with zeros. Guarantees
Copy link
Member Author

@nicholaspai nicholaspai Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Something to verify for the reviewer: We want to guarantee that any caller of depositV3 will never produce a uint256 depositId that could collide with one produced by a caller of unsafeDepositV3. I think this is true because numberOfDeposits is a uint32 while the way unsafeDepositV3 produces a depositId is by packing the 20 byte address of the msg.sender with the uint96 depositId which guarantees that the resultant 32 byte value when casted into a uint256 will always be greater than the MAX_UINT32 if the address is non-0

UPDATED: packing the address with a depositId input by the caller and then hashing the resultant bytes. The chance that a resultant hash collides with a uint32 is less than the chance that the first 28 bytes of the hash are equal to 0 which is extremely small. I don't think this is a realistic possibility we need to guard against.

// that the 24 most significant bytes are 0.
numberOfDeposits++,
quoteTimestamp,
fillDeadline,
uint32(currentTime) + exclusivityPeriod,
uint32(getCurrentTime()) + exclusivityPeriod,
message
);
}

/**
* @notice See depositV3 for details. This function is identical to depositV3 except that it does not use the
* global deposit ID counter as a deposit nonce, instead allowing the caller to pass in a deposit nonce. This
* function is designed to be used by anyone who wants to pre-compute their resultant relay data hash, which
* could be useful for filling a deposit faster and avoiding any risk of a relay hash unexpectedly changing
* due to another deposit front-running this one and incrementing the global deposit ID counter.
* @dev This is labeled "unsafe" because there is no guarantee that the depositId is unique which means that the
* corresponding fill might collide with an existing relay hash on the destination chain SpokePool,
* which would make this deposit unfillable. In this case, the depositor would subsequently receive a refund
* of `inputAmount` of `inputToken` on the origin chain after the fill deadline.
* @dev This is slightly more gas efficient than the depositV3() function because it doesn't
* increment the global deposit count.
* @dev On the destination chain, the hash of the deposit data will be used to uniquely identify this deposit, so
* modifying any params in it will result in a different hash and a different deposit. The hash will comprise
* all parameters to this function along with this chain's chainId(). Relayers are only refunded for filling
* deposits with deposit hashes that map exactly to the one emitted by this contract.
* @param depositNonce The nonce that uniquely identifies this deposit. This function will combine this parameter
* with the msg.sender address to create a unique uint256 depositNonce and ensure that the msg.sender cannot
Copy link

Choose a reason for hiding this comment

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

if the deposit is being made via a 4337 bundler, then the msg.sender will be the bundler address?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not super familiar with 4337 but that sounds right. I think that might be a feature that allows the bundler to ensure that its own deposit hashes are all unique. The alternative instead of msg.sender is using the depositor address but that allows the msg.sender to manipulate the hashes and try to front-run honest msg.senders.

* use this function to front-run another depositor's unsafe deposit. This function guarantees that the resultant
* deposit nonce will not collide with a safe uint256 deposit nonce whose 24 most significant bytes are always 0.
* @param depositor See identically named parameter in depositV3() comments.
* @param recipient See identically named parameter in depositV3() comments.
* @param inputToken See identically named parameter in depositV3() comments.
* @param outputToken See identically named parameter in depositV3() comments.
* @param inputAmount See identically named parameter in depositV3() comments.
* @param outputAmount See identically named parameter in depositV3() comments.
* @param destinationChainId See identically named parameter in depositV3() comments.
* @param exclusiveRelayer See identically named parameter in depositV3() comments.
* @param quoteTimestamp See identically named parameter in depositV3() comments.
* @param fillDeadline See identically named parameter in depositV3() comments.
* @param exclusivityPeriod See identically named parameter in depositV3() comments.
* @param message See identically named parameter in depositV3() comments.
*/
function unsafeDepositV3(
address depositor,
address recipient,
address inputToken,
address outputToken,
uint256 inputAmount,
uint256 outputAmount,
uint256 destinationChainId,
address exclusiveRelayer,
uint96 depositNonce,
uint32 quoteTimestamp,
uint32 fillDeadline,
uint32 exclusivityPeriod,
bytes calldata message
) public payable nonReentrant unpausedDeposits {
// @dev Create the uint256 deposit ID by concatenating the address (which is 20 bytes) with the 12 byte
// depositNonce parameter. We're then left with a 32 byte number if we keccak256 the resultant packed bytes.
// This guarantees the resultant ID won't collide with a safe deposit ID which is set by
// implicitly casting a uint32 to a uint256, whose left-most 24 bytes are 0, while we're guaranteed that
// some of this deposit hash's first 24 bytes are not 0 due to the left shift of the msg.sender address, which
// won't be the zero address.

// @dev For some L2's, it could be possible that the zero address can be the msg.sender which might
// make it possible for an unsafe deposit hash to collide with a safe deposit nonce. To prevent these cases,
// we might want to consider explicitly checking that the msg.sender is not the zero address. However,
// this is unlikely and we also should consider not checking it and incurring the extra gas cost:
// if (msg.sender == address(0)) revert InvalidUnsafeDepositor();

Copy link
Contributor

Choose a reason for hiding this comment

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

OOC, what's the reasoning behind not wrapping all this in a hash? Simplicity for the integrator to predict the deposit hash?

Hash advantages:

  • We can use full 256 bit depositNonces (might improve cross compatibility with things like permit2 or other noncing systems that may be used concurrently).
  • No matter how we concat the data, there is no concern about creating collisions with normal depositIds.
  • We could add more parameters that get baked into the depositId. For instance, this could be called by a contract that passes its msg.sender as the depositor. Adding the depositor to the depositId hash would mean that different depositors wouldn't collide.

Copy link
Member Author

Choose a reason for hiding this comment

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

there is no concern about creating collisions with normal depositIds.

Is this true? What if the hash produces some value where the first 28 bytes are 0 (miraculously)? Wouldn't that then translate into a valid uint32 value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise I totally agree with all the reasons FOR hashing this

Copy link
Contributor

@mrice32 mrice32 Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Did the math on this.

The probability of finding 28 bytes of leading zeros is 1 - (1 - (1/2^224))^num_tries.

You get to about 0.3% after 1e65 tries (screenshot below). For reference, Bitcoin produces about 2e28 hashes per year, meaning it would take about 5e36 years for the entire bitcoin network to have a 0.3% chance of cracking this.

Screenshot 2024-09-26 at 10 26 23 AM

uint256 depositId = uint256(keccak256(abi.encodePacked(msg.sender, depositNonce)));
_depositV3(
depositor,
recipient,
inputToken,
outputToken,
inputAmount,
outputAmount,
destinationChainId,
exclusiveRelayer,
depositId,
quoteTimestamp,
fillDeadline,
uint32(getCurrentTime()) + exclusivityPeriod,
message
);
}
Expand Down Expand Up @@ -754,7 +796,7 @@ abstract contract SpokePool is
*/
function speedUpV3Deposit(
address depositor,
uint32 depositId,
uint256 depositId,
uint256 updatedOutputAmount,
address updatedRecipient,
bytes calldata updatedMessage,
Expand Down Expand Up @@ -1069,6 +1111,77 @@ abstract contract SpokePool is
* INTERNAL FUNCTIONS *
**************************************/

function _depositV3(
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be copied from the existing depositV3 logic

address depositor,
address recipient,
address inputToken,
address outputToken,
uint256 inputAmount,
uint256 outputAmount,
uint256 destinationChainId,
address exclusiveRelayer,
uint256 depositId,
uint32 quoteTimestamp,
uint32 fillDeadline,
uint32 exclusivityDeadline,
bytes calldata message
) internal {
// Check that deposit route is enabled for the input token. There are no checks required for the output token
// which is pulled from the relayer at fill time and passed through this contract atomically to the recipient.
if (!enabledDepositRoutes[inputToken][destinationChainId]) revert DisabledRoute();

// Require that quoteTimestamp has a maximum age so that depositors pay an LP fee based on recent HubPool usage.
// It is assumed that cross-chain timestamps are normally loosely in-sync, but clock drift can occur. If the
// SpokePool time stalls or lags significantly, it is still possible to make deposits by setting quoteTimestamp
// within the configured buffer. The owner should pause deposits/fills if this is undesirable.
// This will underflow if quoteTimestamp is more than depositQuoteTimeBuffer;
// this is safe but will throw an unintuitive error.

// slither-disable-next-line timestamp
uint256 currentTime = getCurrentTime();
if (currentTime - quoteTimestamp > depositQuoteTimeBuffer) revert InvalidQuoteTimestamp();

// fillDeadline is relative to the destination chain.
// Don’t allow fillDeadline to be more than several bundles into the future.
// This limits the maximum required lookback for dataworker and relayer instances.
// Also, don't allow fillDeadline to be in the past. This poses a potential UX issue if the destination
// chain time keeping and this chain's time keeping are out of sync but is not really a practical hurdle
// unless they are significantly out of sync or the depositor is setting very short fill deadlines. This latter
// situation won't be a problem for honest users.
if (fillDeadline < currentTime || fillDeadline > currentTime + fillDeadlineBuffer) revert InvalidFillDeadline();

// If the address of the origin token is a wrappedNativeToken contract and there is a msg.value with the
// transaction then the user is sending the native token. In this case, the native token should be
// wrapped.
if (inputToken == address(wrappedNativeToken) && msg.value > 0) {
if (msg.value != inputAmount) revert MsgValueDoesNotMatchInputAmount();
wrappedNativeToken.deposit{ value: msg.value }();
// Else, it is a normal ERC20. In this case pull the token from the caller as per normal.
// Note: this includes the case where the L2 caller has WETH (already wrapped ETH) and wants to bridge them.
// In this case the msg.value will be set to 0, indicating a "normal" ERC20 bridging action.
} else {
// msg.value should be 0 if input token isn't the wrapped native token.
if (msg.value != 0) revert MsgValueDoesNotMatchInputAmount();
IERC20Upgradeable(inputToken).safeTransferFrom(msg.sender, address(this), inputAmount);
}

emit V3FundsDeposited(
inputToken,
outputToken,
inputAmount,
outputAmount,
destinationChainId,
depositId,
quoteTimestamp,
fillDeadline,
exclusivityDeadline,
depositor,
recipient,
exclusiveRelayer,
message
);
}

function _deposit(
address depositor,
address recipient,
Expand Down Expand Up @@ -1195,7 +1308,7 @@ abstract contract SpokePool is

function _verifyUpdateV3DepositMessage(
address depositor,
uint32 depositId,
uint256 depositId,
uint256 originChainId,
uint256 updatedOutputAmount,
address updatedRecipient,
Expand Down
12 changes: 6 additions & 6 deletions contracts/interfaces/V3SpokePoolInterface.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ interface V3SpokePoolInterface {
// Origin chain id.
uint256 originChainId;
// The id uniquely identifying this deposit on the origin chain.
uint32 depositId;
uint256 depositId;
// The timestamp on the destination chain after which this deposit can no longer be filled.
uint32 fillDeadline;
// The timestamp on the destination chain after which any relayer can fill the deposit.
Expand Down Expand Up @@ -102,7 +102,7 @@ interface V3SpokePoolInterface {
uint256 inputAmount,
uint256 outputAmount,
uint256 indexed destinationChainId,
uint32 indexed depositId,
uint256 indexed depositId,
uint32 quoteTimestamp,
uint32 fillDeadline,
uint32 exclusivityDeadline,
Expand All @@ -114,7 +114,7 @@ interface V3SpokePoolInterface {

event RequestedSpeedUpV3Deposit(
uint256 updatedOutputAmount,
uint32 indexed depositId,
uint256 indexed depositId,
address indexed depositor,
address updatedRecipient,
bytes updatedMessage,
Expand All @@ -128,7 +128,7 @@ interface V3SpokePoolInterface {
uint256 outputAmount,
uint256 repaymentChainId,
uint256 indexed originChainId,
uint32 indexed depositId,
uint256 indexed depositId,
uint32 fillDeadline,
uint32 exclusivityDeadline,
address exclusiveRelayer,
Expand All @@ -145,7 +145,7 @@ interface V3SpokePoolInterface {
uint256 inputAmount,
uint256 outputAmount,
uint256 indexed originChainId,
uint32 indexed depositId,
uint256 indexed depositId,
uint32 fillDeadline,
uint32 exclusivityDeadline,
address exclusiveRelayer,
Expand Down Expand Up @@ -189,7 +189,7 @@ interface V3SpokePoolInterface {

function speedUpV3Deposit(
address depositor,
uint32 depositId,
uint256 depositId,
uint256 updatedOutputAmount,
address updatedRecipient,
bytes calldata updatedMessage,
Expand Down
53 changes: 53 additions & 0 deletions test/evm/hardhat/SpokePool.Deposit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,28 @@ describe("SpokePool Depositor Logic", async function () {
_relayData.message,
];
}
function getUnsafeDepositArgsFromRelayData(
_relayData: V3RelayData,
_depositId: string,
_destinationChainId = destinationChainId,
_quoteTimestamp = quoteTimestamp
) {
return [
_relayData.depositor,
_relayData.recipient,
_relayData.inputToken,
_relayData.outputToken,
_relayData.inputAmount,
_relayData.outputAmount,
_destinationChainId,
_relayData.exclusiveRelayer,
_depositId,
_quoteTimestamp,
_relayData.fillDeadline,
_relayData.exclusivityDeadline,
_relayData.message,
];
}
beforeEach(async function () {
relayData = {
depositor: depositor.address,
Expand Down Expand Up @@ -576,6 +598,37 @@ describe("SpokePool Depositor Logic", async function () {
"ReentrancyGuard: reentrant call"
);
});
it("unsafe deposit ID", async function () {
const currentTime = (await spokePool.getCurrentTime()).toNumber();
// new deposit ID should be the uint256 equivalent of the keccak256 hash of packed {address, forcedDepositId}.
const forcedDepositId = "99";
const expectedDepositId = ethers.utils.solidityKeccak256(
["address", "uint96"],
[depositor.address, forcedDepositId]
);
// console.log(result.toString(), BigNumber.from(result))
await expect(
spokePool
.connect(depositor)
.unsafeDepositV3(...getUnsafeDepositArgsFromRelayData({ ...relayData }, forcedDepositId))
)
.to.emit(spokePool, "V3FundsDeposited")
.withArgs(
relayData.inputToken,
relayData.outputToken,
relayData.inputAmount,
relayData.outputAmount,
destinationChainId,
BigNumber.from(expectedDepositId),
quoteTimestamp,
relayData.fillDeadline,
currentTime,
depositor.address,
relayData.recipient,
relayData.exclusiveRelayer,
relayData.message
);
});
});
describe("speed up V3 deposit", function () {
const updatedOutputAmount = amountToDeposit.add(1);
Expand Down
2 changes: 1 addition & 1 deletion test/evm/hardhat/fixtures/SpokePool.Fixture.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ export async function getUpdatedV3DepositSignature(
const typedData = {
types: {
UpdateDepositDetails: [
{ name: "depositId", type: "uint32" },
{ name: "depositId", type: "uint256" },
{ name: "originChainId", type: "uint256" },
{ name: "updatedOutputAmount", type: "uint256" },
{ name: "updatedRecipient", type: "address" },
Expand Down
Loading