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

refactor: ImmutableERC1155 threat model #229

Merged
merged 17 commits into from
Jun 17, 2024

Conversation

allan-almeida-imtbl
Copy link
Contributor

@allan-almeida-imtbl allan-almeida-imtbl commented Jun 13, 2024

Update threat model for ImmutableERC1155 contract:

  • Contract inheritance diagram & descriptions
  • Document externally visible functions & access control
  • Various formatting

@allan-almeida-imtbl allan-almeida-imtbl marked this pull request as ready for review June 13, 2024 05:40

Contracts covered under this model include:

- [ImmutableERC1155](../../contracts/token/erc1155/preset/ImmutableERC1155.sol)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have this link to a specific githash. Given you are using the diagram from the README, it is conceivable that the diagram may change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have updated this link and also links for underlying contracts, to keep them all in sync: cef4175

Functions that _change_ state:
| Name | Function Selector | Access Control |
| ------------------------------------------------------------- | ----------------- | --------------------- |
| burn(address,uint256,uint256) | f5298aca | None - permisionless |
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect the access control is that only the token owner can call burn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, have addressed all state changing functions that previously mentioned being permissionless

ipekt
ipekt previously approved these changes Jun 17, 2024
@allan-almeida-imtbl allan-almeida-imtbl requested a review from a team as a code owner June 17, 2024 05:32
@allan-almeida-imtbl allan-almeida-imtbl merged commit fbf54a8 into main Jun 17, 2024
6 of 7 checks passed
@allan-almeida-imtbl allan-almeida-imtbl deleted the refactor/GMS-1614/erc1155-threat-model branch June 17, 2024 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants