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

Update > to >= on collectTokens #70

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oovg
Copy link

@oovg oovg commented Mar 24, 2020

As long as there's no security risk, this would allow collectTokens to work in the following example:

One sends and ERC20 straight to the DAO's contract.
DAO wants to use the ERC20, so can whitelist the token through a proposal.
IF passes, any DAO member can collectTokens to sync the balance.

Currently, due to the > instead of >= a tribute must be offered through a proposal first to raise the balance over 0 in order to collectTokens.

As long as there's no security risk, this would allow collectTokens to work in the following example:

One sends and ERC20 straight to the DAO's contract.
DAO wants to use the ERC20, so can whitelist the token through a proposal.
IF passes, any DAO member can collectTokens to sync the balance.

Currently, due to the `>` instead of `>=` a tribute must be offered through a proposal first to raise the balance over `0` in order to collectTokens.
@HeikoFisch
Copy link
Contributor

Hey, and thanks for your input!

This would indeed simplify the situation you describe, but (as you have alluded to as a possibility) there are security-related implications in other scenarios:

With the convention of calling a token active if it has non-zero GUILD balance, we currently have a limit of 200 active and 400 whitelisted tokens. These limits ensure we don't run out of gas during ragequits and -kicks, both of which require us to iterate over all tokens.
The rationale for two different limits is that inactive tokens are very cheap in the aforementioned loop, because there are no funds in this token to actually transfer to the quitter/kickee.
So we can have considerably more whitelisted tokens than active ones, which allows the DAO members to swap out a token that, for some reason or other, has become less interesting for another one that currently seems more attractive.

Now, if we allowed any member to (re-)activate a token by calling collectTokens (send a tiny amount of that token to the contract first), then an attacker could possibly reactivate many tokens in quick succession, finally reaching the limit of 200 active tokens, and thereby interfere with the DAO's regular operation.
Therefore, we chose to restrict collectTokens to already active ones, so that any token (re-)activation has to go through the regular proposal and voting process.

We briefly discussed allowing non-whitelisted tokens in collectTokens (so you'd only need a whitelist proposal, not a whitelist and a tribute proposal to get them in), but if the token never got whitelisted, the funds would be stuck in the contract, so you'd probably want to get it whitelisted first anyway. In the end, it was decided to accept the slight inconvenience you describe.

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 this pull request may close these issues.

None yet

2 participants