aragon / osx

Aragon OSx Protocol
GNU Affero General Public License v3.0
85 stars 43 forks source link

fix tokenvoting ivotes checks #600

Closed novaknole closed 1 week ago

novaknole commented 4 months ago

Description

Why this PR ?

This PR addresses the following problem: some users, even though they were using OZ's ERC20Votes, they didn't have ERC165(supportsInterface function) implemented in their contracts. This caused the problem where our TokenVotingSetup always would wrap such a token and the token applied on TokenVoting would be a wrapped one even though there was no need for that. Note that even though token would be wrapped, that wouldn't be a critical issue or cause any bricking because people would just call approve + depositFor and still maintain the same features. The problem I'm referring is that they would need to have extra layer of calling approve/depositFor which is not ideal and not only that, they were confused why the wrapping was necessary.

Does the solution solve it ?

There're 2 ways to solve this problem.

How the checks work:

// token = address(0) => deploy governanceERC20
// token != address(0)
//    token == IVote => do nothing and use this token directly
//    token != IVote => we wrap

---

// If token is IVote, it's either wrapped or not. We don't care which one is it because it already supports "getPastVotes, getVotes, getPastTotalSupply" functions.
// If token is not IVote, that means those 3 functions together are not supported by token. So we must wrap.

---

// If the token only has 2 out of those 3 functions, we would wrap. It might be the case that some contracts don't use all 
// these 3 functions, but only 2 of them, but problem why we would still need to wrap the token is that `TokenVoting` on 
// which token will be applied to, uses all these 3 functions. If we don't wrap, token will be applied but 
// TokenVoting will not work correctly since when it calls the function(not present in token), it will always fail.  This is why 
// we would always wrap unless all these 3 functions are present on the token - ("getPastVotes, getVotes, 
// getPastTotalSupply"). The scenario 
// when 2 out of 3 or 1 out of 3 are only present in token would happen if token deployer has no idea what he is doing or 
// doesn't follow OZ standards at all. In either case, UI will detect if not all 3 is present and if so, will give a warning to the 
// user that if he still submits this tx, his token will be wrapped which user will be confused about.

Why making this PR to main branch ?

If we want to have this fix before 1.4.0 is audited, then we can't use TokenVotingPlugin repo(post-split) since this repo uses 1.4.0. So we have no choice but to use main branch on osx. It would be great though NOT to merge this PR, because if we do, main branch will contain this new fix code whereas in audit report(by hellborn) didn't sign on this. It's a question of whether we want to have the non-audited code on main branch. I'd say no and go for the option that we leave this code on this PR, audit internally and anyways release it.

Can Old tokens that were wrapped incorrectly be fixed ?

The only fix is to add update code inside TokenVotingSetup that calls initializeV2 on the tokenVoting and passes new/fixed token. This has so many problems as if the proposal is already created on TokenVoting with old token, it must continue using old one and new proposals must use new tokens, but still, tokens that were wrapped incorrectly already use the funds on it and if we deploy new tokens, those funds won't be on this token. Long story short, this needs much,much deeper thoughts.

One more change in prepareUninstallation

We think that there's no need to revoke MINT_PERMISSION on the token contract for dao. Why ? Even though plugin(tokenvoting) was uninstalled, token still should continue to be operated by dao as it kind of belongs to dao. Think about it - if we still revoke this permission, nobody will have MINT_PERMISSION on the token and token becomes useless(only in minting case) even though it has funds ? We still think dao still should continue operating it. - Please think about this as we're not sold on either ideas.

Last thoughts

I haven't implemented the tests as I would first like to get a grasp of what you all thinking.

CLAassistant commented 4 months ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Giorgi Lagidze seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.