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
Fix spend allowance overloading #368
Merged
0xdcota
merged 2 commits into
protocol/fix/macro-findings-critical-high
from
protocol/fix/spend-allowance-overloading
Mar 22, 2023
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fuji contracts allow users to increase/decrease their withdrawal allowance per operator, per receiver through various methods of vault permissions.
fuji-v2/packages/protocol/src/vaults/VaultPermissions.sol
Lines 94 to 108 in efed334
Hence it's not always given that
receiver == operator
Which contradicts this PR
If an allowance is set for a different receiver than the operator, this pr would always spend the allowance for the same operator only.
Either consider restricting operator= receiver only in all allowance methods or refractor this fix to accommodate operator!=receiver case.
One way of doing the latter is overriding
transferFrom
method directly and using_spendWithdrawAllowance
in place of_spendAllowance
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/1642b6639b93e3b97be163d49827e1f56b81ca11/contracts/token/ERC20/ERC20.sol#L160
owner is from
spender is operator
to is receiver
amount needs to be converted to assets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is three ways to modify the '_withdrawAllowance` state defined in the VaultPermissions contract:
approve
method, required by EIP20, that we override at the BaseVault level. Approve assumesoperator
=receiver
because we have no option. You cannot change the approve method signature and we don't want to have a "dual" approval system. We need to force all approvals to be handled by '_withdrawAllowance` state defined in the VaultPermissions contract.increaseAllowance
ordecreaseAllowance
which we override at the BaseVault so that, again, there is no "dual" approval system. Since the signature is already defined we then have to assumeoperator
=receiver
. If there was a way to delete methods from a parent contract we would just delete these two. They are not official part of the EIP20.increaseWithdrawAllowance
ordecreaseWithdrawAllowance
which includes the parameter sperately foroperator
andreceiver
. This is what we want users to "default".I hope this clarifies. In the mean time I did some small refactors, including to consider the msg.sender in
transferFrom
as the "operator". Refer to changes in #536