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

Boson voucher owner should not be able to renounceOwnership #588

Closed
zajck opened this issue Mar 21, 2023 · 2 comments · Fixed by #591
Closed

Boson voucher owner should not be able to renounceOwnership #588

zajck opened this issue Mar 21, 2023 · 2 comments · Fixed by #591
Assignees
Labels
bug Something isn't working v2.2.0

Comments

@zajck
Copy link
Member

zajck commented Mar 21, 2023

Description:

BosonVoucher inherits OwnableUpgradeable which implements method renounceOwnership(). If owner calls it, it loses its owner privileges, therefore it can no longer call methods with onlyOwner modifier. Affected functions are preMint, burnPremintedVouchers, setContractURI, callExternalContract and setApprovalForAllToContract.

If that happens, seller is not totally broken, since updating assistant address in the protocol also sets it as a new contract owner. Therefore this is then merely an inconvenience that can happen. It is still better to prevent it.

Also, since transferOwnership is restricted (owner cannot call it directly) it does not make sense to leave a similar function unrestricted. Most likely renouncing ownership should be part of acount deactivation inside the protocol.

Recommendation:

Override renounceOwnership() in BosonVoucher and revert if it is called.

@zajck zajck added the bug Something isn't working label Mar 21, 2023
@mischat
Copy link
Member

mischat commented Mar 21, 2023

I agree with this, especially given that updating the assistant will always impact the ownership of the vouchers, probably worth just making this action do nothing for now.

@mischat
Copy link
Member

mischat commented Mar 21, 2023

Good find

@zajck zajck self-assigned this Mar 23, 2023
zajck added a commit that referenced this issue Mar 23, 2023
@zajck zajck added the v2.2.0 label Mar 30, 2023
zajck added a commit that referenced this issue Apr 19, 2023
Co-authored-by: Mischa <[email protected]>
anajuliabit pushed a commit that referenced this issue Apr 21, 2023
* Alternative tokenId

* fix tests

* move id handling to reserve range

* fix integration

* set min exchange id in constructor

* fix deploy

* fix unit tests

* review fixes

* remove extractExchageId + optimize vouchers

* remove transferpremintedFrom

* Fix #587

* rename seller -> rangeOwner

* Change balance check

* fix old test

* Fix #588

* Refactor tests A-D

* Refactor tests E-M

* Refactor tests O-T

* use util functions in upgrade tests

* Refactor integration tests

* Fix failing tests

* Refactor client tests

* added missing accountIds

* simplify: no facetNames required anymore

* Improved chunking algorithm

* Cover uncovered branches

* add missing tests + config

* CI: coverage

* set _committed in issueVoucher

* uncomment line

---------

Co-authored-by: Mischa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working v2.2.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants