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

Remove onlyOwner from accept[] functions #98

Closed
MerlinEgalite opened this issue Sep 21, 2023 · 6 comments · Fixed by #145
Closed

Remove onlyOwner from accept[] functions #98

MerlinEgalite opened this issue Sep 21, 2023 · 6 comments · Fixed by #145
Assignees

Comments

@MerlinEgalite
Copy link
Contributor

          That makes me think that should the accept timelock be `onlyOwner`?

Originally posted by @MerlinEgalite in #96 (comment)

@Rubilmax
Copy link
Contributor

In the case of the timelock, the fee & the guardian, I think it is not necessary, but it doesn't make that much sense to remove it either

Whereas for caps, the owner may change their mind during the timelock period for some market-related reasons and they wouldn't be able to cancel it

More generally, the timelock adds a time delay that can make assumptions previously considered true when submitting a change now be false (market volatility for example)
Removing onlyOwner makes impossible to roll back a submitted change, which is a bold move and we need to make sure there's 0 case where a time-dependent assumption is involved

@MerlinEgalite
Copy link
Contributor Author

and they wouldn't be able to cancel it

This is because of the current implementation but that could be updated. We could delete the pending value when submitting the current value for example.

I don't have a strong opinion on this feature though

@Rubilmax
Copy link
Contributor

Rubilmax commented Sep 21, 2023

We could delete the pending value when submitting the current value for example.

This is actually what we do: the pending value gets overridden everytime a value is submitted
But in practice, an owner changing their mind could get frontrunned (and would, if it is economically interesting), hence why I'm stating that in practice, the owner "wouldn't be able to cancel it"

@Rubilmax
Copy link
Contributor

What shall we do?

@MerlinEgalite MerlinEgalite closed this as not planned Won't fix, can't repro, duplicate, stale Sep 25, 2023
@julien-devatom
Copy link
Contributor

The owner already submits the pending value, so why restrict the acceptance of the value after the timelock to the owner?

Removing the onlyOwner modifier permits automating the acceptance of the values, or using of a multicall to batch acceptances, without more security risks.

The consideration of the frontrunning is just after the timelock. The owner has timelock s to change his mind, and the duration must be chosen according to this spec IMO. It is way cleaner for vault users to prepare an allocation and to be sure that, after the timelock, the allocation is possible. Without onlyOwner, the timelock is the exact time after considering the allocation as true. With onlyOwner, the timelock is the minimal time after considering the allocation as true. This is more deterministic to not use onlyOwner.

@julien-devatom julien-devatom reopened this Oct 3, 2023
@Rubilmax
Copy link
Contributor

Rubilmax commented Oct 3, 2023

The owner has timelock s to change his mind, and the duration must be chosen according to this spec IMO

This is what changed my mind
And the fact that allocators would be able to instantly accept new values and submit allocations consequently

@Rubilmax Rubilmax changed the title Should acceptFunction be onlyOwner? Remove onlyOwner from accept[] functions Oct 3, 2023
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 a pull request may close this issue.

3 participants