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

[Bug]: immutable controllerGasLimit enforced with GAS opcode #739

Closed
wjmelements opened this issue Jun 5, 2024 · 8 comments
Closed

[Bug]: immutable controllerGasLimit enforced with GAS opcode #739

wjmelements opened this issue Jun 5, 2024 · 8 comments
Labels
bug Something isn't working

Comments

@wjmelements
Copy link
Contributor

Describe the bug

It is unclear why the controllerGasLimit design was chosen.

As far as I can tell, the immutable controllerGasLimit is a bad mechanism. The gas costs of opcodes are not fixed and can change with a protocol upgrade. Hard forks can and have increased gas costs. This gasleft mechanism is not future-proof and is worse than a simpler design.

Expected Behavior

Revert if protocolFeesForPool fails.

To Reproduce

No response

Additional context

No response

@wjmelements wjmelements added the bug Something isn't working label Jun 5, 2024
Copy link

linear bot commented Jun 5, 2024

@hensha256
Copy link
Contributor

This was introduced to fix #221. We would rather a call to the protocol fee controller ran out of gas, and the protocol fee initialzied to 0, than pools be unable to initialize.

is worse than a simpler design.

What simpler design did you have in mind? We are open to changing it for a design that fixes both issues!

@wjmelements
Copy link
Contributor Author

Revert if protocolFeesForPool fails.

@wjmelements
Copy link
Contributor Author

Bad Owner Can Brick initialize By Setting A Bad protocolFeeController

Why not accept this behavior?

@wjmelements
Copy link
Contributor Author

Alternatively, it should be made more explicit that the owner can globally shut down the creation of new pools via the initialize function.

This should have been the solution.

@wjmelements
Copy link
Contributor Author

In fact the original problem still persists because

            _checkProtocolFee(protocolSwapFee);
            _checkProtocolFee(protocolWithdrawFee);

happens outside of the catch. So they can still return invalid protocol fees to block initialize.

@hensha256
Copy link
Contributor

I think youre looking at an old branch maybe? Theres no try / catch in the contract, and theres no protocolWithdrawFee nor _checkProtocolFee.

Do you mind looking at main and seeing if you still find an issue?

@wjmelements
Copy link
Contributor Author

You are right. I was looking at 6b8ec7c. The current version is better than what I was looking at.

I suppose I am okay with the protocol fees going to zero in the event of gas cost increases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants