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

[protocol review] Check enabled market in reallocate withdraw loop #219

Closed
QGarchery opened this issue Oct 18, 2023 · 5 comments · Fixed by #236
Closed

[protocol review] Check enabled market in reallocate withdraw loop #219

QGarchery opened this issue Oct 18, 2023 · 5 comments · Fixed by #236
Assignees

Comments

@QGarchery
Copy link
Contributor

The reallocate does not check that the withdrawn markets are in the withdrawQueue. This means that assets could come from unknown parties (legal problem ?) not whitelisted by the curator, potentially after someone has supplied on behalf of the vault on those markets. It also means that the idle amount could be inflated with amounts that were not supplied on the vault

@Rubilmax
Copy link
Contributor

That is correct and it's a known behavior. I think the allocator can be held responsible for such actions. Is it an issue? Should it be documented?

@QGarchery
Copy link
Contributor Author

Re-opening this because I don't understand why we would want that behavior. This is in the same vein as #218: basically it's the last safety measure to ensure that allocators cannot interact with markets that have not been previously approved by the curator. The check should only be about checking that the withdrawRank of the market is not 0

@QGarchery QGarchery reopened this Oct 27, 2023
@MerlinEgalite
Copy link
Contributor

It seems there's no legal reason to do it and it does not seem to be a requirement either

@QGarchery
Copy link
Contributor Author

it does not seem to be a requirement either

There is almost no requirements in metamorpho roles, because basically you can say that everyone is trusted. In this case you can question the utility to having different roles. But I think the distinction between the allocators and the curator is very powerful, and allows metamorpho vault admins to sleep at peace using bots as allocators.

It goes like this "allocators are restricted to markets approved by the curator/owner" and we are literally one line of code away from it

@Rubilmax Rubilmax closed this as not planned Won't fix, can't repro, duplicate, stale Nov 17, 2023
@QGarchery
Copy link
Contributor Author

To provide a little bit more context on why this was closed as not planned: now that there is a way to "force remove" a market (maybe because it is broken), it becomes convenient to be able to rescue funds without having to wait for the timelock. Additionally, putting the cap above 0 on a market that is scheduled for removal is awkward because it potentially leaves a window for users to supply on that market before it is actually removed

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