Skip to content

Conversation

GWSzeto
Copy link
Contributor

@GWSzeto GWSzeto commented Sep 20, 2024

Addresses the changes found in the audit prelim report for modular contracts

@GWSzeto GWSzeto force-pushed the audit-fixes branch 2 times, most recently from 70b53eb to a351d1a Compare September 20, 2024 18:11
…tokenId when installed after initial minting

- updated sequentialTokenIdERC1155 to now be able to initialize nextTokenId
…ens with decimals other than 18

- uses IERC20.decimals instead of 1e18
@GWSzeto GWSzeto force-pushed the audit-fixes branch 2 times, most recently from c8030b9 to 2adab3b Compare September 28, 2024 18:59
//////////////////////////////////////////////////////////////*/

/// @dev Returns bytes encoded install params, to be sent to `onInstall` function
function encodeBytesOnInstall(uint256 nextTokenId) external pure returns (bytes memory) {
Copy link
Member

Choose a reason for hiding this comment

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

Would call this startTokenId to be clear of what it is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

uint256 nextTokenIdRangeStart;
// tokenId range end => baseURI of range
mapping(uint256 => string) baseURIOfTokenIdRange;
// tokenId range end
Copy link
Member

Choose a reason for hiding this comment

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

Update comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

(uint256 startTokenId,) = getBatchRange(_batchId);
emit BatchMetadataUpdate(startTokenId, _batchId);
/// @dev Sets the base URI for the batch based on the batchIndex.
function setBaseURI(uint256 _batchIndex, string memory _baseURI) external virtual {
Copy link
Member

Choose a reason for hiding this comment

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

Would help me to have a list of ABI changes, hard to see from this pr

@GWSzeto GWSzeto merged commit be6f40a into main Oct 9, 2024
5 checks passed
@GWSzeto GWSzeto deleted the audit-fixes branch October 9, 2024 15:59
GWSzeto added a commit that referenced this pull request Nov 7, 2024
* [L-4] Wrong function selector returned for the transfer validation function (#150)

* [L-4] Wrong function selector returned for the transfer validation function

* [Q-3] Move interface identifier for ERC165 to Core

* [Q-5] Royalty modules should inherit ICreatorToken interface

* [Q-6] Nitpicks

* removes duplicate supportsInterface (#156)

* Has roles (#158)

* created hasRole function in the roylaty module

* created tests

* Merge dev to main (#163)

* renamed to nextTokenIdToMint in BatchMetadata (#141)

* fix: expectRevert on low-level call (#136)

* update batchMetadata logic to make each batchUris independent (#144)

* update batchMetadata logic to make each batchUris independent

* format

* Module Core Refactor (#145)

* separated out metadata functionality from mintable module

* built in signature mint into the core erc721 contract

* implemented in Mintable

* Implemented updateMetadata

* simplified parameters and structs

* all tests pass

* updated ERC721 initializable to match ERC721Core

* addressed the PR issues

* updated 1155 versions to now match 721 implementations

* completed all the tests

* Implemented parity in the ERC1155Initializable contract

* unified naming from quantity and value to amount

* slapped on a keccak256

* move OwnableRoles check on the signature

* removed double events being emitted

* tests pass

* updated ERC20 core

* implemented Claimable and Mintable on the ERC20 side

* tests pass

* updated based on PR feedback

* Fix getSupportedCallbackFunctions ub ERC721CoreInitializable (#149)

* implmented delayed functionality into batchMetadata (#148)

* implmented delayed functionality into batchMetadata

* created tests for BatchMetadata

* updated ERC1155 tests and updated from batchStartId to batchRange

* Implement tokenIdERC1155 module to handle tokenId management (#147)

* initial commit

* created tests for tokenIdERC1155

* updated to be optional

* updated naming and tests

* Transfer validator has roles (#143)

* created hasRole function in the roylaty module

* created tests

* Optimzed callback execution (#135)

* gas benchmark

* optimize execute callback function

* fix typo

* optimize execute callback view function

* optimize callback mode loop

* Implement Max per wallet (#151)

* implemented maxMintPerWallet

* tests pass

* maxMintPerWalletExceeded tests pass

* introduced base contracts for core and initilizable to inherit

* rename commit

* rename commit

* renamed from core to coreInitializable for the ERC1155 (#152)

* updated to now use 1e18 divided (#153)

* Remove double initializer in ERC721CoreInitializable (#154)

* rebase off of main (#157)

* [L-4] Wrong function selector returned for the transfer validation function (#150)

* [L-4] Wrong function selector returned for the transfer validation function

* [Q-3] Move interface identifier for ERC165 to Core

* [Q-5] Royalty modules should inherit ICreatorToken interface

* [Q-6] Nitpicks

* removes duplicate supportsInterface (#156)

* renamed to nextTokenIdToMint in BatchMetadata (#141)

* fix: expectRevert on low-level call (#136)

* update batchMetadata logic to make each batchUris independent (#144)

* update batchMetadata logic to make each batchUris independent

* format

* Module Core Refactor (#145)

* separated out metadata functionality from mintable module

* built in signature mint into the core erc721 contract

* implemented in Mintable

* Implemented updateMetadata

* simplified parameters and structs

* all tests pass

* updated ERC721 initializable to match ERC721Core

* addressed the PR issues

* updated 1155 versions to now match 721 implementations

* completed all the tests

* Implemented parity in the ERC1155Initializable contract

* unified naming from quantity and value to amount

* slapped on a keccak256

* move OwnableRoles check on the signature

* removed double events being emitted

* tests pass

* updated ERC20 core

* implemented Claimable and Mintable on the ERC20 side

* tests pass

* updated based on PR feedback

* Fix getSupportedCallbackFunctions ub ERC721CoreInitializable (#149)

* implmented delayed functionality into batchMetadata (#148)

* implmented delayed functionality into batchMetadata

* created tests for BatchMetadata

* updated ERC1155 tests and updated from batchStartId to batchRange

* Implement tokenIdERC1155 module to handle tokenId management (#147)

* initial commit

* created tests for tokenIdERC1155

* updated to be optional

* updated naming and tests

* Transfer validator has roles (#143)

* created hasRole function in the roylaty module

* created tests

* Optimzed callback execution (#135)

* gas benchmark

* optimize execute callback function

* fix typo

* optimize execute callback view function

* optimize callback mode loop

* Implement Max per wallet (#151)

* implemented maxMintPerWallet

* tests pass

* maxMintPerWalletExceeded tests pass

* introduced base contracts for core and initilizable to inherit

* rename commit

* rename commit

* renamed from core to coreInitializable for the ERC1155 (#152)

* updated to now use 1e18 divided (#153)

* Remove double initializer in ERC721CoreInitializable (#154)

---------

Co-authored-by: Pranav Garg <[email protected]>
Co-authored-by: Joaquim Verges <[email protected]>

* Duplicate constant (#161)

* [L-4] Wrong function selector returned for the transfer validation function (#150)

* [L-4] Wrong function selector returned for the transfer validation function

* [Q-3] Move interface identifier for ERC165 to Core

* [Q-5] Royalty modules should inherit ICreatorToken interface

* [Q-6] Nitpicks

* removes duplicate supportsInterface (#156)

* Has roles (#158)

* created hasRole function in the roylaty module

* created tests

* removed duplicate constant in royaltyERC721

* removed .vscode

* case sensitivity issue

---------

Co-authored-by: Pranav Garg <[email protected]>
Co-authored-by: Joaquim Verges <[email protected]>

* added hasRole to creatorTokenERC20 module (#167)

* Audit fixes (#173)

* [M-1] The SequentialTokenIdERC1155 module fails to apply the correct tokenId when installed after initial minting

- updated sequentialTokenIdERC1155 to now be able to initialize nextTokenId

* [H-1] ClaimableERC20 and MintableERC20 modules incorrectly handle tokens with decimals other than 18

- uses IERC20.decimals instead of 1e18

* [H-2] Claimable modules lead to storage collisions when being updgraded

* [H-3] BatchMetadata modules may apply baseURI to incorrect token ids

* [Q-1] FallbackFunction array of Claimable modules can be reduced

* [Q-2] Nitpicks

---------

Co-authored-by: Pranav Garg <[email protected]>
Co-authored-by: Joaquim Verges <[email protected]>
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