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

feat: reject both input and output fees #247

Merged
merged 1 commit into from
Apr 25, 2024
Merged

Conversation

marktoda
Copy link
Collaborator

this avoids a double-fee case from protocol fee controller

this avoids a double-fee case from protocol fee controller
@marktoda marktoda force-pushed the reject-input-and-output-fees branch from 150d28c to 1d0a95c Compare April 19, 2024 17:17
@@ -67,13 +71,17 @@ abstract contract ProtocolFees is Owned {
for (uint256 j = 0; j < outputsLength; j++) {
OutputToken memory output = order.outputs[j];
if (output.token == feeOutput.token) {
if (inputFeeTaken) revert InputAndOutputFees();
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe im misreading but this code will never be true since inputFeeTaken is assigned after this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, it's inside the outside loop. basically:

for every fee output added:

  • is it a fee-on-output? (loop thru all outputs to check)
  • is it a fee-on-input?

and this is just disallowing both, so placement inside the outer loop doesnt matter

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah I see

@marktoda marktoda requested a review from zhongeric April 25, 2024 11:02
@@ -67,13 +71,17 @@ abstract contract ProtocolFees is Owned {
for (uint256 j = 0; j < outputsLength; j++) {
OutputToken memory output = order.outputs[j];
if (output.token == feeOutput.token) {
if (inputFeeTaken) revert InputAndOutputFees();
Copy link
Collaborator

Choose a reason for hiding this comment

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

ah I see

@marktoda marktoda merged commit a5d7abf into main Apr 25, 2024
3 checks passed
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.

None yet

2 participants