Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix/controller swap fees #80

Merged
merged 7 commits into from
Apr 1, 2024
Merged

Conversation

clemlak
Copy link
Contributor

@clemlak clemlak commented Mar 19, 2024

@clemlak clemlak self-assigned this Mar 19, 2024
@clemlak clemlak added 📃 contracts Anything related to the DFMM contracts (or strategies) 🔨 audit fix Patching audit findings labels Mar 19, 2024
src/DFMM.sol Outdated
Comment on lines 241 to 242
uint256 fees =
state.deltaLiquidity.mulWadDown(_pools[poolId].controllerFee);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use mulWadUp, which will only make fees == 0 when deltaLiquidity == 0, then we can just try to avoid deltaLiquidity == 0 in the other parts of the code to avoid the fees == 0 code branch

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, should we be checking if controllerFee > 0 in the if?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe there's a case where you want to burn liquidity (dumb idea ik), in which case you might want non-zero controllerFee but feeCollector == address(0)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe there's a case where you want to burn liquidity (dumb idea ik), in which case you might want non-zero controllerFee but feeCollector == address(0)?

Yeah, I think it makes sense to check the controller fee in the if condition instead of the address.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to prevent swap from burning liquidity we can still reintroduce the address check later.

@clemlak clemlak changed the base branch from main to fix/spearbit-audit March 25, 2024 10:07
@clemlak clemlak requested review from Alexangelj and kinrezC March 29, 2024 07:00
@clemlak clemlak merged commit 86b6b0b into fix/spearbit-audit Apr 1, 2024
5 checks passed
@clemlak clemlak deleted the fix/controller-swap-fees branch April 1, 2024 07:16
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🔨 audit fix Patching audit findings 📃 contracts Anything related to the DFMM contracts (or strategies)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants