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

Add crowdloan rewards to non-transfer proxy #1048

Merged
merged 3 commits into from
Dec 2, 2021

Conversation

girazoki
Copy link
Collaborator

@girazoki girazoki commented Dec 2, 2021

What does it do?

Adds Crowdloan rewards to non-transfer proxy

What important points reviewers should know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

@girazoki girazoki added the B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes label Dec 2, 2021
@crystalin crystalin added the A0-pleasereview Pull request needs code review. label Dec 2, 2021
@girazoki girazoki marked this pull request as ready for review December 2, 2021 14:07
Copy link
Contributor

@JoshOrndorff JoshOrndorff left a comment

Choose a reason for hiding this comment

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

This PR allows Proxy accounts of type NonTransfer to interact with the crowdloan rewards pallet on behalf of the proxied account. The primary user-facing benefit of this change is that the proxy accounts can claim rewards. However, it also provides access to all other crowdloan rewards extrinsics including associate_native_identity, and change_rewards_address.

I believe providing access to change_reward_address is a security hole. With this change, a proxy account could redirect unclaimed rewards to their own account, thereby stealing the rewards from the proxied account.

Copy link
Contributor

@JoshOrndorff JoshOrndorff left a comment

Choose a reason for hiding this comment

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

We could add a test ensuring that the proxy cannot call any other extrinsics in the pallet. But I'm fine doing that in a followup.

@girazoki
Copy link
Collaborator Author

girazoki commented Dec 2, 2021

I can do that, does not cost me anything now that I have the basic test in

@librelois librelois mentioned this pull request Dec 2, 2021
15 tasks
@girazoki girazoki merged commit 623f74d into master Dec 2, 2021
@girazoki girazoki deleted the girazoki-crowdloan-rewards-through-proxies branch December 2, 2021 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants