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

H-01 SpokePool's fill Function Performs Malformed Call #744

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bmzig
Copy link
Contributor

@bmzig bmzig commented Nov 11, 2024

The fill function of the SpokePool contract is meant to adhere to the IDestinationSettler interface, as dictated by the latest update to the ERC-7683 specifications. The fill function is meant to internally call the fillV3Relay function in order to process the order data, and it does so by making a delegatecall to its own fillV3Relay function, passing abi.encodePacked(originData, fillerData) as the parameter.

However, the fillV3Relay function accepts two parameters, having the repaymentChainId as the second parameter. Since the call is constructed using encodeWithSelector, which is not type-safe, the compiler does not complain about the missing parameter. As an incorrect number of parameters is passed, the call to fillV3Relay will always revert when trying to decode the input parameters, breaking the entire execution flow. Moreover, the input data is encoded with abi.encodePacked the use of which is discouraged, especially when dealing with structs and dynamic types like arrays.

Consider using encodeCall instead of encodeWithSelector to ensure type safety, and providing the parameters required by the fillV3Relay function separately. In addition, consider explicitly making the SpokePool contract inherit from the IDestinationSettler interface as required by the ERC-7683 standard.

This PR performs the suggested change and decodes the input data to fields of fillV3Relay, so that abi.encodeCall can be used.

@bmzig bmzig marked this pull request as ready for review November 14, 2024 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants