feat: [POST AUDIT] periphery contracts #1052
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* feat(SpokePoolPeriphery): Support multiple exchanges Currently we can only initialize the periphery contract with a single exchange to swap with. This PR allows us to initialize it with multiple exchanges to swap with. Like before, these initial set of exchanges and function selectors cannot be changed post-initialization, which gives the user assurances. * rename * Update SpokeV3PoolPeriphery.sol * Update SpokeV3PoolPeriphery.sol * Update SpokeV3PoolPeriphery.sol * Add unit tests * Add whitelistExchanges only owner method * rename * Remove onlyOwner * Remove whitelist of exchanges, add proxy to bypass approval abuse Make user approve proxy contract so no one can use `exchange` + `routerCalldata` to steal their already approved funds via the `SpokePoolPeriphery` * Add some protection to callSpokePoolPeriphery * Only call swapAndBridge through proxy * move periphery funcs into proxy * Update SpokePoolV3Periphery.sol * remove depositERC20 * Update SpokePoolV3Periphery.sol * Add back safeTransferFron's to permit funcs * Add unit tests that check if calling deposit and swapAndBridge with no value fails directly * Add interfaces to make sure we don't add new functions as easily * Add Create2Factory * feat: add permit2 entrypoints to the periphery (#782) * feat: add permit2 entrypoints to the periphery Signed-off-by: Bennett <[email protected]> * Update test/evm/foundry/local/SpokePoolPeriphery.t.sol * Update SpokePoolPeriphery.t.sol * move permit2 to proxy * fix permit2 Signed-off-by: bennett <[email protected]> * wip: swap arguments refactor Signed-off-by: bennett <[email protected]> * implement isValidSignature Signed-off-by: bennett <[email protected]> * 1271 Signed-off-by: bennett <[email protected]> * simplify isValidSignature Signed-off-by: bennett <[email protected]> * rebase /programs on master Signed-off-by: nicholaspai <[email protected]> * clean up comments * rebase programs * fix: consolidate structs so that permit2 witnesses cover inputs Signed-off-by: bennett <[email protected]> * begin permit2 unit tests Signed-off-by: bennett <[email protected]> * rebase * Update SpokePoolPeriphery.t.sol * move type definitions to interface Signed-off-by: bennett <[email protected]> * fix permit2 test Signed-off-by: bennett <[email protected]> * transfer type tests Signed-off-by: bennett <[email protected]> * rename EIP1271Signature to Permi2Approval Signed-off-by: bennett <[email protected]> --------- Signed-off-by: Bennett <[email protected]> Signed-off-by: bennett <[email protected]> Signed-off-by: nicholaspai <[email protected]> Co-authored-by: nicholaspai <[email protected]> Co-authored-by: nicholaspai <[email protected]> * feat: sponsored swap and deposits (#790) * feat: add permit2 entrypoints to the periphery Signed-off-by: Bennett <[email protected]> * Update test/evm/foundry/local/SpokePoolPeriphery.t.sol * Update SpokePoolPeriphery.t.sol * move permit2 to proxy * fix permit2 Signed-off-by: bennett <[email protected]> * wip: swap arguments refactor Signed-off-by: bennett <[email protected]> * implement isValidSignature Signed-off-by: bennett <[email protected]> * 1271 Signed-off-by: bennett <[email protected]> * simplify isValidSignature Signed-off-by: bennett <[email protected]> * rebase /programs on master Signed-off-by: nicholaspai <[email protected]> * clean up comments * rebase programs * feat: sponsored swap and deposits Signed-off-by: bennett <[email protected]> * fix: consolidate structs so that permit2 witnesses cover inputs Signed-off-by: bennett <[email protected]> * begin permit2 unit tests Signed-off-by: bennett <[email protected]> * rebase * Update SpokePoolPeriphery.t.sol * move type definitions to interface Signed-off-by: bennett <[email protected]> * fix permit2 test Signed-off-by: bennett <[email protected]> * transfer type tests Signed-off-by: bennett <[email protected]> * rename EIP1271Signature to Permi2Approval Signed-off-by: bennett <[email protected]> * add mockERC20 which implements permit/receiveWithAuthorization Signed-off-by: bennett <[email protected]> * add tests for permit, permit2, and receiveWithAuth swaps/deposits Signed-off-by: bennett <[email protected]> * add tests for invalid witnesses Signed-off-by: bennett <[email protected]> * factor out signature checking Signed-off-by: bennett <[email protected]> --------- Signed-off-by: Bennett <[email protected]> Signed-off-by: bennett <[email protected]> Signed-off-by: nicholaspai <[email protected]> Co-authored-by: nicholaspai <[email protected]> Co-authored-by: nicholaspai <[email protected]> * feat: Delete SwapAndBridge and add submission fees to gasless flow (#809) * feat: add permit2 entrypoints to the periphery Signed-off-by: Bennett <[email protected]> * Update test/evm/foundry/local/SpokePoolPeriphery.t.sol * Update SpokePoolPeriphery.t.sol * move permit2 to proxy * fix permit2 Signed-off-by: bennett <[email protected]> * wip: swap arguments refactor Signed-off-by: bennett <[email protected]> * implement isValidSignature Signed-off-by: bennett <[email protected]> * 1271 Signed-off-by: bennett <[email protected]> * simplify isValidSignature Signed-off-by: bennett <[email protected]> * rebase /programs on master Signed-off-by: nicholaspai <[email protected]> * clean up comments * rebase programs * feat: sponsored swap and deposits Signed-off-by: bennett <[email protected]> * fix: consolidate structs so that permit2 witnesses cover inputs Signed-off-by: bennett <[email protected]> * begin permit2 unit tests Signed-off-by: bennett <[email protected]> * rebase * Update SpokePoolPeriphery.t.sol * move type definitions to interface Signed-off-by: bennett <[email protected]> * fix permit2 test Signed-off-by: bennett <[email protected]> * transfer type tests Signed-off-by: bennett <[email protected]> * rename EIP1271Signature to Permi2Approval Signed-off-by: bennett <[email protected]> * add mockERC20 which implements permit/receiveWithAuthorization Signed-off-by: bennett <[email protected]> * add tests for permit, permit2, and receiveWithAuth swaps/deposits Signed-off-by: bennett <[email protected]> * add tests for invalid witnesses Signed-off-by: bennett <[email protected]> * feat: Delete SwapAndBridge and add submission fees to gasless flow SwapAndBridge is to be replaced with SpokePoolV3Periphery Gasless flows will require user to cover gas cost of whoever submits the transaction, but they can be set to 0 if the user wants to submit themselves. * Internal refactor * Update SpokePoolV3Periphery.sol * Update PeripherySigningLib.sol * Update SpokePoolV3Periphery.sol * Update PeripherySigningLib.sol --------- Signed-off-by: Bennett <[email protected]> Signed-off-by: bennett <[email protected]> Signed-off-by: nicholaspai <[email protected]> Co-authored-by: Bennett <[email protected]> * Update SpokePoolV3Periphery.sol * Update SpokePoolPeriphery.t.sol * Move all comments to interface and use inherit doc * fix: eip712 types and hashes (#821) * refactor comments Signed-off-by: bennett <[email protected]> * Create IERC20Auth.sol * fix tests * Comments --------- Signed-off-by: Bennett <[email protected]> Signed-off-by: bennett <[email protected]> Signed-off-by: nicholaspai <[email protected]> Co-authored-by: bmzig <[email protected]> Co-authored-by: Bennett <[email protected]> Co-authored-by: Dong-Ha Kim <[email protected]>
…min (#998) * feat: add proportional adjustment to outputAmount Signed-off-by: Matt Rice <[email protected]> * add option to disable output adjustment Signed-off-by: Matt Rice <[email protected]> * format Signed-off-by: Matt Rice <[email protected]> * WIP Signed-off-by: Matt Rice <[email protected]> --------- Signed-off-by: Matt Rice <[email protected]>
* feat: add proportional adjustment to outputAmount Signed-off-by: Matt Rice <[email protected]> * add option to disable output adjustment Signed-off-by: Matt Rice <[email protected]> * format Signed-off-by: Matt Rice <[email protected]> * WIP Signed-off-by: Matt Rice <[email protected]> * fix: fix Nick's comment Signed-off-by: Matt Rice <[email protected]> --------- Signed-off-by: Matt Rice <[email protected]>
* feat: add balance replacements for MulticallHandler Signed-off-by: Matt Rice <[email protected]> * WIP Signed-off-by: Matt Rice <[email protected]> * WIP Signed-off-by: Matt Rice <[email protected]> * WIP Signed-off-by: Matt Rice <[email protected]> --------- Signed-off-by: Matt Rice <[email protected]>
…1002) * WIP Signed-off-by: Matt Rice <[email protected]> * WIP Signed-off-by: Matt Rice <[email protected]> * fix adjustment Signed-off-by: Matt Rice <[email protected]> * make non evm compatible, use deposit method Signed-off-by: Matt Rice <[email protected]> * comments Signed-off-by: Matt Rice <[email protected]> * one more comment Signed-off-by: Matt Rice <[email protected]> * remove stray comments Signed-off-by: Matt Rice <[email protected]> * remove unnecessary variable Signed-off-by: Matt Rice <[email protected]> * remove errors Signed-off-by: Matt Rice <[email protected]> * nit and type string Signed-off-by: bennett <[email protected]> * WIP Signed-off-by: Matt Rice <[email protected]> * Remove factory, remove initialize, remove wrappedNativeToken and spokePool from storage Signed-off-by: Matt Rice <[email protected]> * Remove repetitive event Signed-off-by: Matt Rice <[email protected]> * event definition too Signed-off-by: Matt Rice <[email protected]> * naming Signed-off-by: Matt Rice <[email protected]> --------- Signed-off-by: Matt Rice <[email protected]> Signed-off-by: bennett <[email protected]> Co-authored-by: bennett <[email protected]>
Signed-off-by: Matt Rice <[email protected]>
* [M-02] Replay Attacks on SpokePoolPeriphery Possible Signed-off-by: Matt Rice <[email protected]> * WIP Signed-off-by: Matt Rice <[email protected]> * WIP Signed-off-by: Matt Rice <[email protected]> --------- Signed-off-by: Matt Rice <[email protected]>
- Remove unused InvalidSignatureLength error from SpokePoolPeriphery.sol - Remove unused IERC20Auth import from SpokePoolPeripheryInterface.sol These changes improve the overall clarity, intentionality, and readability of the codebase by removing currently unused code.
Fix several typographical errors throughout the codebase: - Fix 'calldData' to 'callData' on line 48 of MulticallHandler.sol - Remove unnecessary 'at' from 'receive funds at on destination chain' on line 113 of SpokePoolPeripheryInterface.sol - Fix 'depositData/swapAndDepositData' to 'DepositData/SwapAndDepositData' on line 500 of SpokePoolPeriphery.sol These corrections improve the clarity and readability of the codebase.
Signed-off-by: Matt Rice <[email protected]>
* [L-05] Inflexible Fee Recipient Field Blocks Open Relaying Currently, every DepositData and SwapAndDepositData payload must include a hard-coded fee recipient address, and upon successful deposit or swap-and-bridge the periphery pays submission fees to that exact address. While this ensures the user knows in advance exactly who will receive their fee, it also prevents open relayer competition or fallback options when the chosen relayer is unavailable or underperforms. This change keeps the explicit fee recipient field option in SwapAndDepositData but introduces a "zero‐address" convention: - If the fee recipient is equal to the zero address, the periphery defaults to using msg.sender as the payee - Otherwise, transfer fees to the signed recipient as today This enables open relayer competition while maintaining backward compatibility. * WIP Signed-off-by: Matt Rice <[email protected]> --------- Signed-off-by: Matt Rice <[email protected]>
The deposit function of the SpokePoolPeriphery contract allows users to deposit native value to the SpokePool. Although it is possible to specify the inputToken parameter, it is not possible to deposit other tokens through this function. Because of that, it could be renamed to depositNative or a similar name in order to make this fact clear. Consider renaming the deposit function in order to improve readability of the codebase.
* M-05 Incorrect EIP-712 Encoding Fix EIP-712 encoding issue by replacing TransferType enum with uint8 in SwapAndDepositData type string and cast enum to uint8 in hash encoding. Add comments to TransferType enum values showing their integer mappings. * Remove unnecessary uint8 cast in EIP-712 encoding The cast to uint8 is redundant since the EIP-712 type string already specifies uint8 transferType, making the encoding identical without the explicit cast.
* [N-12] Misleading Documentation Throughout the codebase, there are several instances of misleading documentation. The examples are listed below. The swapAndBridgeWithPermit and depositWithPermit functions are documented to fail if the provided token does not support the EIP-2612 permit function. However, the implementation contradicts this statement as in both functions, the call to permit is wrapped in a try/catch block, and any failure is silently ignored. The comment refers to the transferWithAuthorization function, while it should mention the receiveWithAuthorization function instead. The documentation of the SpokePoolPeriphery contract and the SpokePoolPeripheryInterface interface contain an outdated comment claiming that certain variables are not marked immutable or set in the constructor to allow deterministic deployment. This is no longer true as the variables are now immutable and set via the constructor. Consider fixing the instances mentioned above in order to enhance the clarity of the codebase. * Clarify permit call documentation Update comment to specify that the permit call result is ignored rather than the call itself when tokens don't implement EIP-2612 permit functionality.
* M-03: Prevent DoS attack by disallowing Permit2 as exchange address * fix test Signed-off-by: Matt Rice <[email protected]> --------- Signed-off-by: Matt Rice <[email protected]>
The deposit function of the SpokePoolPeriphery contract allows users to deposit native value to the SpokePool. However, its recipient and exclusiveRelayer arguments are both of type address and are then casted to bytes32. As a result, it is not possible to bridge wrapped native tokens to non-EVM blockchains. Consider changing the type of the recipient and exclusiveRelayer arguments of the deposit function so that callers are allowed to specify non-EVM addresses for deposits.
Add documentation to all external swap-and-bridge functions explaining the potential overflow case when depositData.outputAmount * returnAmount exceeds 2^256-1 during proportional adjustment calculations.
* [N-03] Optimization Opportunities Throughout the codebase, there are several places where the code could be optimized in order to save gas. The examples are: The checks validating that a given address refers to a contract in lines 204, 231 and 553 are not necessary as in case when the addresses do not refer to contracts, the subsequent calls at lines 207, 233 and 555 will revert as the Solidity compiler inserts similar code size checks before each high-level call. The "0x" string passed to permit call could be replaced with "". The check could be removed as the same check is already performed in SpokePools. This would additionally allow users to deposit non-native tokens through the SpokePoolPeriphery.deposit function. The replacement argument of the makeCallWithBalance function could be stored in calldata instead of memory. The use of the Lockable contract is inefficient. OpenZeppelin's ReentrancyGuard delivers significantly lower gas overhead by using a two‐word uint256 status in place of a bool, reducing SSTORE costs, and swapping long revert strings for a 4-byte custom error to shrink both bytecode and revert gas. For deployments on chains that support EIP-1153 (transient storage), adopting ReentrancyGuardTransient can further nearly eliminate reentrancy‐guard gas costs. Consider applying the suggestions above in order to provide more gas efficient code. * WIP Signed-off-by: Matt Rice <[email protected]> * optimize: store replacement array in calldata instead of memory Changes the replacement parameter in makeCallWithBalance function from memory to calldata for gas optimization since the array is only read from and never modified. * fix test Signed-off-by: Matt Rice <[email protected]> * fix tests Signed-off-by: Matt Rice <[email protected]> --------- Signed-off-by: Matt Rice <[email protected]>
mrice32
changed the title
mrice32
changed the title
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
nicholaspai
approved these changes
Jun 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The code in this PR has been audited, with a public report forthcoming.