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

feat(protocol): change to transfer-and-burn pattern with NFT vaults #17049

Merged
merged 11 commits into from
May 9, 2024

Conversation

adaki2004
Copy link
Contributor

To remove additional trust assumption, and do not allow vaults (if compromised) to arbitrarily burn tokens. Adapt the ‘transfer and burn’ pattern for NFTs as well (as we did for ERC20).

Copy link

openzeppelin-code bot commented May 8, 2024

feat(protocol): change to transfer-and-burn pattern with NFT vaults

Generated at commit: 67b945110d6c2a618f37be83f83f9613f8c236f6

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
2
2
0
6
41
51
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

@adaki2004 adaki2004 force-pushed the vault_burn_design_fix branch from 1080072 to 6e73966 Compare May 8, 2024 14:44
@dantaik dantaik requested review from KorbinianK and cyberhorsey May 8, 2024 14:45
@dantaik
Copy link
Contributor

dantaik commented May 8, 2024

@KorbinianK @cyberhorsey I think you guys need to know about this change (similar to what we did to BridgedERC20 vaults). Now you always have to get approvals first before burning bridged NFTs, just like what you do with canonical NFTs.

@adaki2004 adaki2004 requested a review from dantaik May 8, 2024 14:47
@adaki2004
Copy link
Contributor Author

@KorbinianK @cyberhorsey I think you guys need to know about this change (similar to what we did to BridgedERC20 vaults). Now you always have to get approvals first before burning bridged NFTs, just like what you do with canonical NFTs.

@KorbinianK
Yes, so for bridged tokens, while bridging back we now require approve() for erc721 and setApprovalForAll() for erc1155.

@dantaik dantaik changed the title feat(protocol): change to transfer and burn pattern with nft vaults feat(protocol): change to transfer-and-burn pattern with NFT vaults May 8, 2024
@dantaik dantaik enabled auto-merge May 9, 2024 03:13
@dantaik dantaik added this pull request to the merge queue May 9, 2024
Merged via the queue into main with commit 22ac9ae May 9, 2024
4 checks passed
@dantaik dantaik deleted the vault_burn_design_fix branch May 9, 2024 06:31
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.

3 participants