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

Extension token locking #922

Merged
merged 8 commits into from
Feb 11, 2021
Merged

Extension token locking #922

merged 8 commits into from
Feb 11, 2021

Conversation

kronosapiens
Copy link
Member

@kronosapiens kronosapiens commented Feb 2, 2021

Closes #921

Supports #915, allowing extensions (here, VotingToken) to lock & unlock user's tokens.

@kronosapiens kronosapiens changed the title Allow network-managed extensions to lock & unlock tokens Extension token locking Feb 2, 2021
@kronosapiens kronosapiens self-assigned this Feb 2, 2021
contracts/colony/ColonyStorage.sol Outdated Show resolved Hide resolved
contracts/colony/ColonyStorage.sol Outdated Show resolved Hide resolved
test/contracts-network/colony-network-extensions.js Outdated Show resolved Hide resolved
contracts/colony/Colony.sol Outdated Show resolved Hide resolved
Copy link
Member

@area area left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, while I think this implementation is now fine, I don't think it can be merged in the current state. The problem is that with the existence of the VotingReputation contract (which can call arbitrary functions on a colony with a root motion), anyone can unlock anyone else's tokens arbitrarily if this function is introduced.

@kronosapiens
Copy link
Member Author

kronosapiens commented Feb 8, 2021

Ah! You're so right. I think the fix here is to add a mapping of [extension][lockId] => bool so you can only unlock the locks you've created.

@area
Copy link
Member

area commented Feb 9, 2021

Also unfortunately, such a mapping cannot be trusted because of recovery mode.

@kronosapiens
Copy link
Member Author

@area I don't think that's a major concern. A lot of things "cannot be trusted" because of recovery mode, but by definition it's a highly trusted mode so I'm not worried. If anything we could call it a feature -- a bug was found in an extension, we need to remove the lock so we "give" the lock to another voting contract, etc. Using recovery mode as part of an attack on a token lock seems like much more trouble than it's worth.

@area area merged commit d2723d7 into develop Feb 11, 2021
@area area deleted the feat/extension-locking branch February 11, 2021 10:50
@area
Copy link
Member

area commented Feb 11, 2021

You've not commented on my changes to the tokenlocking event or the requireExecuteCall changes so I assume you're good with them 😛

@kronosapiens
Copy link
Member Author

Definitely :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow network-managed extensions to lock tokens
2 participants