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

unchecked slot0 interface #24

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

Conversation

thedavidmeister
Copy link

When I pointed the quoter at non canonical AMMs that implement uniswap v2/v3 broadly, I found an instance of minor non-compliance that makes it impossible to get a quote.

Specifically I found that pancake swap returns protocol fees as uint32 rather than uint8 in slot0.

The quoter doesn't actually use any of the non-compliant data, and as calldata abi encodes all primitives as 32 byte values regardless of the uint bit size, there should be no danger in accepting uint256 values for anything that is discarded by the quoter anyway.

The issue is that even if the quoter never uses the data, it is decoded and checked for overflows by Solidity according to the interface anyway.

Perhaps this could be considered a bug in Solidity, that ignored data is a revert condition, or perhaps this is desired behaviour upstream as it implies the interfaces differ (which they do in this case, but in a harmless way I believe).

This PR implements a loose version of the slot0 call that accepts uint256 for all values that the quoter ignores anyway.

This is a much bigger change than the minimum required to support pancake swap, which would just be a move from uint8 to uint32 on the protocol fee. The reason I set all these to uint256 is that I didn't want to overfit for pancake swap specifically. In the scheme of things I don't really care about pancake swap, I'm more interested in being able to use the quoter in as many situations as possible.

@thedavidmeister
Copy link
Author

@jsy1218 is this waiting on something? it was approved nearly 3 months ago

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