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

[5/n permissions] feat: add erc20 token limit plugin #72

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

howydev
Copy link
Collaborator

@howydev howydev commented Jun 16, 2024

No description provided.

@howydev howydev force-pushed the howy/add-usdc-token-limit-plugin branch from 04f7b56 to a8b4b8b Compare June 17, 2024 21:15
@howydev howydev force-pushed the howy/add-native-token-limit-plugin branch 2 times, most recently from 224b9d6 to 3da0f37 Compare June 17, 2024 21:22
@howydev howydev force-pushed the howy/add-usdc-token-limit-plugin branch from a8b4b8b to f8bc912 Compare June 17, 2024 21:22
@howydev howydev force-pushed the howy/add-native-token-limit-plugin branch from 3da0f37 to 9bb72f9 Compare June 17, 2024 22:48
@howydev howydev force-pushed the howy/add-usdc-token-limit-plugin branch from f8bc912 to b957137 Compare June 17, 2024 22:49
@howydev howydev force-pushed the howy/add-native-token-limit-plugin branch from 9bb72f9 to df3870d Compare June 17, 2024 23:34
@howydev howydev force-pushed the howy/add-usdc-token-limit-plugin branch from b957137 to 32d5467 Compare June 17, 2024 23:34
@howydev howydev force-pushed the howy/add-native-token-limit-plugin branch from df3870d to f0f72e0 Compare June 18, 2024 17:52
@howydev howydev force-pushed the howy/add-usdc-token-limit-plugin branch from 32d5467 to dcd5cd4 Compare June 18, 2024 17:52
@howydev howydev changed the title [DRAFT] [5/n permissions] feat: add erc20 token limit plugin [5/n permissions] feat: add erc20 token limit plugin Jun 18, 2024
@howydev howydev marked this pull request as ready for review June 18, 2024 21:27
Copy link
Contributor

@adamegyed adamegyed left a comment

Choose a reason for hiding this comment

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

Same as #71 , quick thoughts for now:

Comment on lines +90 to +91
executeCalldata.offset := add(relativeOffset, 32)
executeCalldata.length := calldataload(relativeOffset)
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 is vulnerable to manipulation because unlike abi.decode, this doesn't check the bounds of the offset or length - you could pass an offset that points to something invalid in the outer calldata, but points to something valid in inner calldata. E.g. the add(...) operations are unchecked, so you could use large numbers to effective subtract an offset, rather than add, and get varying behavior between the checking plugin and the actual receiving token contract.

Might be fine for now with some warnings, or we could just switch these to be abi.decode instead. As a sample, we don't have to worry too much about gas.

if (spend > limit) {
revert ExceededNativeTokenLimit();
}
limits[msg.sender][token][functionId] = limit - spend;
Copy link
Contributor

Choose a reason for hiding this comment

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

This linter warning is curious - I couldn't find an external function invoked before _checkAndDecrementLimit. Do you see any?

@howydev howydev force-pushed the howy/add-native-token-limit-plugin branch from f0f72e0 to 942de3c Compare June 19, 2024 19:49
@howydev howydev force-pushed the howy/add-usdc-token-limit-plugin branch from dcd5cd4 to 547b99a Compare June 19, 2024 19:49
Base automatically changed from howy/add-native-token-limit-plugin to howy/remove-permissions June 19, 2024 20:00
@howydev howydev merged commit 547b99a into howy/remove-permissions Jun 19, 2024
3 checks passed
@howydev howydev deleted the howy/add-usdc-token-limit-plugin branch June 19, 2024 20:01
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