Skip to content

Commit

Permalink
Merge pull request #1145 from thesandboxgame/asset-audit-tsb-01
Browse files Browse the repository at this point in the history 
Shorten revert messages and update tests
  • Loading branch information
@atkinsonholly
atkinsonholly authored Sep 18, 2023
2 parents 1ce2167 + f7d7fae commit 9eb8a70
Show file tree
Hide file tree
Showing 11 changed files with 76 additions and 84 deletions.
3 changes: 1 addition & 2 deletions packages/asset/.solhint.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
}
],
"compiler-version": ["error", "^0.8.0"],
"func-visibility": ["error", {"ignoreConstructors": true}],
"reason-string": ["warn", {"maxLength": 64}]
"func-visibility": ["error", {"ignoreConstructors": true}]
}
}
17 changes: 7 additions & 10 deletions packages/asset/contracts/Asset.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,8 @@ contract Asset is
uint256[] memory amounts,
string[] memory metadataHashes
) external onlyRole(MINTER_ROLE) {
require(ids.length == metadataHashes.length, "Asset: ids and metadataHash length mismatch");
require(ids.length == amounts.length, "Asset: ids and amounts length mismatch");
require(ids.length == metadataHashes.length, "Asset: 1-Array mismatch");
require(ids.length == amounts.length, "Asset: 2-Array mismatch");
for (uint256 i = 0; i < ids.length; i++) {
_setMetadataHash(ids[i], metadataHashes[i]);
}
Expand Down Expand Up @@ -193,7 +193,7 @@ contract Asset is
/// @param metadataHash The metadata hash to set
function _setMetadataHash(uint256 tokenId, string memory metadataHash) internal {
if (hashUsed[metadataHash] != 0) {
require(hashUsed[metadataHash] == tokenId, "Asset: not allowed to reuse metadata hash");
require(hashUsed[metadataHash] == tokenId, "Asset: Hash already used");
} else {
hashUsed[metadataHash] = tokenId;
_setURI(tokenId, metadataHash);
Expand All @@ -204,7 +204,7 @@ contract Asset is
/// @dev Change the address of the trusted forwarder for meta-TX
/// @param trustedForwarder The new trustedForwarder
function setTrustedForwarder(address trustedForwarder) external onlyRole(DEFAULT_ADMIN_ROLE) {
require(trustedForwarder != address(0), "Asset: trusted forwarder can't be zero address");
require(trustedForwarder != address(0), "Asset: Zero address");
_setTrustedForwarder(trustedForwarder);
}

Expand Down Expand Up @@ -294,10 +294,7 @@ contract Asset is
uint256 amount,
bytes memory data
) public virtual override onlyAllowedOperator(from) {
require(
from == _msgSender() || isApprovedForAll(from, _msgSender()),
"ERC1155: caller is not token owner or approved"
);
require(from == _msgSender() || isApprovedForAll(from, _msgSender()), "Asset: Transfer error");
_safeTransferFrom(from, to, id, amount, data);
}

Expand Down Expand Up @@ -363,14 +360,14 @@ contract Asset is
external
onlyRole(DEFAULT_ADMIN_ROLE)
{
require(subscriptionOrRegistrantToCopy != address(0), "Asset: subscription can't be zero address");
require(subscriptionOrRegistrantToCopy != address(0), "Asset: Zero address");
_registerAndSubscribe(subscriptionOrRegistrantToCopy, subscribe);
}

/// @notice sets the operator filter registry address
/// @param registry the address of the registry
function setOperatorRegistry(address registry) external onlyRole(DEFAULT_ADMIN_ROLE) {
require(registry != address(0), "Asset: registry can't be zero address");
require(registry != address(0), "Asset: Zero address");
OperatorFiltererUpgradeable._setOperatorFilterRegistry(registry);
}

Expand Down
2 changes: 1 addition & 1 deletion packages/asset/contracts/AssetCreate.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ contract AssetCreate is
/// @dev Change the address of the trusted forwarder for meta-TX
/// @param trustedForwarder The new trustedForwarder
function setTrustedForwarder(address trustedForwarder) external onlyRole(DEFAULT_ADMIN_ROLE) {
require(trustedForwarder != address(0), "AssetCreate: trusted forwarder can't be zero address");
require(trustedForwarder != address(0), "AssetCreate: Zero address");
_setTrustedForwarder(trustedForwarder);
}

Expand Down
32 changes: 16 additions & 16 deletions packages/asset/contracts/AssetReveal.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,14 +114,14 @@ contract AssetReveal is
string[] calldata metadataHashes,
bytes32[] calldata revealHashes
) external whenNotPaused {
require(amounts.length == metadataHashes.length, "AssetReveal: Invalid amounts length");
require(amounts.length == revealHashes.length, "AssetReveal: Invalid revealHashes length");
require(amounts.length == metadataHashes.length, "AssetReveal: 1-Array mismatch");
require(amounts.length == revealHashes.length, "AssetReveal: 2-Array mismatch");
require(
authValidator.verify(
signature,
_hashReveal(_msgSender(), prevTokenId, amounts, metadataHashes, revealHashes)
),
"AssetReveal: Invalid revealMint signature"
"AssetReveal: Invalid signature"
);
uint256[] memory newTokenIds = _revealAsset(prevTokenId, metadataHashes, amounts, revealHashes);
emit AssetRevealMint(_msgSender(), prevTokenId, amounts, newTokenIds, revealHashes);
Expand All @@ -141,15 +141,15 @@ contract AssetReveal is
string[][] calldata metadataHashes,
bytes32[][] calldata revealHashes
) external whenNotPaused {
require(prevTokenIds.length == amounts.length, "AssetReveal: Invalid amounts length");
require(amounts.length == metadataHashes.length, "AssetReveal: Invalid metadataHashes length");
require(prevTokenIds.length == revealHashes.length, "AssetReveal: Invalid revealHashes length");
require(prevTokenIds.length == amounts.length, "AssetReveal: 1-Array mismatch");
require(amounts.length == metadataHashes.length, "AssetReveal: 2-Array mismatch");
require(prevTokenIds.length == revealHashes.length, "AssetReveal: 3-Array mismatch");
require(
authValidator.verify(
signature,
_hashBatchReveal(_msgSender(), prevTokenIds, amounts, metadataHashes, revealHashes)
),
"AssetReveal: Invalid revealBatchMint signature"
"AssetReveal: Invalid signature"
);
uint256[][] memory newTokenIds = new uint256[][](prevTokenIds.length);
for (uint256 i = 0; i < prevTokenIds.length; i++) {
Expand All @@ -174,16 +174,16 @@ contract AssetReveal is
string[] calldata metadataHashes,
bytes32[] calldata revealHashes
) external whenNotPaused {
require(amounts.length == metadataHashes.length, "AssetReveal: Invalid amounts length");
require(amounts.length == revealHashes.length, "AssetReveal: Invalid revealHashes length");
require(amounts.length == metadataHashes.length, "AssetReveal: 1-Array mismatch");
require(amounts.length == revealHashes.length, "AssetReveal: 2-Array mismatch");
uint8 tier = prevTokenId.getTier();
require(tierInstantRevealAllowed[tier], "AssetReveal: Tier not allowed for instant reveal");
require(tierInstantRevealAllowed[tier], "AssetReveal: Not allowed");
require(
authValidator.verify(
signature,
_hashInstantReveal(_msgSender(), prevTokenId, amounts, metadataHashes, revealHashes)
),
"AssetReveal: Invalid burnAndReveal signature"
"AssetReveal: Invalid signature"
);
_burnAsset(prevTokenId, burnAmount);
uint256[] memory newTokenIds = _revealAsset(prevTokenId, metadataHashes, amounts, revealHashes);
Expand All @@ -202,7 +202,7 @@ contract AssetReveal is
) internal returns (uint256[] memory) {
uint256[] memory newTokenIds = getRevealedTokenIds(metadataHashes, prevTokenId);
for (uint256 i = 0; i < revealHashes.length; i++) {
require(revealHashesUsed[revealHashes[i]] == false, "AssetReveal: RevealHash already used");
require(revealHashesUsed[revealHashes[i]] == false, "AssetReveal: Hash already used");
revealHashesUsed[revealHashes[i]] = true;
}
if (newTokenIds.length == 1) {
Expand Down Expand Up @@ -231,8 +231,8 @@ contract AssetReveal is

function _verifyBurnData(uint256 tokenId, uint256 amount) internal pure {
IAsset.AssetData memory data = tokenId.getData();
require(!data.revealed, "AssetReveal: Asset is already revealed");
require(amount > 0, "AssetReveal: Burn amount should be greater than 0");
require(!data.revealed, "AssetReveal: Already revealed");
require(amount > 0, "AssetReveal: Invalid amount");
}

/// @notice Creates a hash of the reveal data
Expand Down Expand Up @@ -373,7 +373,7 @@ contract AssetReveal is
returns (uint256[] memory tokenIds)
{
IAsset.AssetData memory data = prevTokenId.getData();
require(!data.revealed, "AssetReveal: already revealed");
require(!data.revealed, "AssetReveal: Already revealed");
uint256[] memory tokenIdArray = new uint256[](metadataHashes.length);
for (uint256 i = 0; i < metadataHashes.length; i++) {
uint256 tokenId = assetContract.getTokenIdByMetadataHash(metadataHashes[i]);
Expand Down Expand Up @@ -438,7 +438,7 @@ contract AssetReveal is
/// @dev Change the address of the trusted forwarder for meta-TX
/// @param trustedForwarder The new trustedForwarder
function setTrustedForwarder(address trustedForwarder) external onlyRole(DEFAULT_ADMIN_ROLE) {
require(trustedForwarder != address(0), "AssetReveal: trusted forwarder can't be zero address");
require(trustedForwarder != address(0), "AssetReveal: Zero address");
_setTrustedForwarder(trustedForwarder);
}

Expand Down
2 changes: 1 addition & 1 deletion packages/asset/contracts/AuthSuperValidator.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ contract AuthSuperValidator is AccessControl {
/// @return bool
function verify(bytes memory signature, bytes32 digest) public view returns (bool) {
address signer = _signers[_msgSender()];
require(signer != address(0), "AuthSuperValidator: signer not set");
require(signer != address(0), "AuthSuperValidator: No signer");
address recoveredSigner = ECDSA.recover(digest, signature);
return recoveredSigner == signer;
}
Expand Down
22 changes: 11 additions & 11 deletions packages/asset/contracts/Catalyst.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,12 @@ contract Catalyst is
string[] memory _catalystIpfsCID,
address _royaltyManager
) external initializer {
require(bytes(_baseUri).length != 0, "Catalyst: base uri can't be empty");
require(_trustedForwarder != address(0), "Catalyst: trusted forwarder can't be zero");
require(_subscription != address(0), "Catalyst: subscription can't be zero");
require(_defaultAdmin != address(0), "Catalyst: admin can't be zero");
require(_defaultMinter != address(0), "Catalyst: minter can't be zero");
require(_royaltyManager != address(0), "Catalyst: royalty manager can't be zero");
require(bytes(_baseUri).length != 0, "Catalyst: URI empty");
require(_trustedForwarder != address(0), "Catalyst: 1-Zero address");
require(_subscription != address(0), "Catalyst: 2-Zero address");
require(_defaultAdmin != address(0), "Catalyst: 3-Zero address");
require(_defaultMinter != address(0), "Catalyst: 4-Zero address");
require(_royaltyManager != address(0), "Catalyst: 5-Zero address");
__ERC1155_init(_baseUri);
__AccessControl_init();
__ERC1155Burnable_init();
Expand Down Expand Up @@ -166,7 +166,7 @@ contract Catalyst is
/// @dev Change the address of the trusted forwarder for meta-TX
/// @param trustedForwarder The new trustedForwarder
function setTrustedForwarder(address trustedForwarder) external onlyRole(DEFAULT_ADMIN_ROLE) {
require(trustedForwarder != address(0), "Catalyst: trusted forwarder can't be zero address");
require(trustedForwarder != address(0), "Catalyst: Zero address");
_setTrustedForwarder(trustedForwarder);
}

Expand All @@ -178,14 +178,14 @@ contract Catalyst is
onlyRole(DEFAULT_ADMIN_ROLE)
onlyValidId(tokenId)
{
require(bytes(metadataHash).length != 0, "Catalyst: metadataHash can't be empty");
require(bytes(metadataHash).length != 0, "Catalyst: Metadata hash empty");
_setURI(tokenId, metadataHash);
}

/// @notice Set a new base URI
/// @param baseURI The new base URI
function setBaseURI(string memory baseURI) external onlyRole(DEFAULT_ADMIN_ROLE) {
require(bytes(baseURI).length != 0, "Catalyst: base uri can't be empty");
require(bytes(baseURI).length != 0, "Catalyst: URI empty");
_setBaseURI(baseURI);
emit BaseURISet(baseURI);
}
Expand Down Expand Up @@ -301,14 +301,14 @@ contract Catalyst is
external
onlyRole(DEFAULT_ADMIN_ROLE)
{
require(subscriptionOrRegistrantToCopy != address(0), "Catalyst: subscription can't be zero address");
require(subscriptionOrRegistrantToCopy != address(0), "Catalyst: Zero address");
_registerAndSubscribe(subscriptionOrRegistrantToCopy, subscribe);
}

/// @notice sets filter registry address
/// @param registry the address of the registry
function setOperatorRegistry(address registry) external onlyRole(DEFAULT_ADMIN_ROLE) {
require(registry != address(0), "Catalyst: registry can't be zero address");
require(registry != address(0), "Catalyst: Zero address");
OperatorFiltererUpgradeable._setOperatorFilterRegistry(registry);
emit OperatorRegistrySet(registry);
}
Expand Down
16 changes: 8 additions & 8 deletions packages/asset/test/Asset.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ describe('Base Asset Contract (/packages/asset/contracts/Asset.sol)', function (
await mintOne(undefined, undefined, undefined, metadataHashes[0]);
await expect(
mintOne(undefined, undefined, undefined, metadataHashes[0])
).to.be.revertedWith('Asset: not allowed to reuse metadata hash');
).to.be.revertedWith('Asset: Hash already used');
});
});
describe('Batch Mint', function () {
Expand Down Expand Up @@ -301,7 +301,7 @@ describe('Base Asset Contract (/packages/asset/contracts/Asset.sol)', function (
metadataHashes[0],
metadataHashes[0],
])
).to.be.revertedWith('Asset: not allowed to reuse metadata hash');
).to.be.revertedWith('Asset: Hash already used');
});
it('should not allow minting with already existing metadata hash', async function () {
const {mintOne, mintBatch, metadataHashes} = await runAssetSetup();
Expand All @@ -311,7 +311,7 @@ describe('Base Asset Contract (/packages/asset/contracts/Asset.sol)', function (
metadataHashes[0],
metadataHashes[1],
])
).to.be.revertedWith('Asset: not allowed to reuse metadata hash');
).to.be.revertedWith('Asset: Hash already used');
});
it("should not allow minting if the length of the ids and amounts don't match", async function () {
const {AssetContractAsMinter, generateRandomTokenId, minter} =
Expand All @@ -330,7 +330,7 @@ describe('Base Asset Contract (/packages/asset/contracts/Asset.sol)', function (
amounts,
hashes
)
).to.be.revertedWith('Asset: ids and amounts length mismatch');
).to.be.revertedWith('Asset: 2-Array mismatch');
});
it("should not allow minting if the length of the ids and hashes don't match", async function () {
const {AssetContractAsMinter, generateRandomTokenId, minter} =
Expand All @@ -345,7 +345,7 @@ describe('Base Asset Contract (/packages/asset/contracts/Asset.sol)', function (
amounts,
hashes
)
).to.be.revertedWith('Asset: ids and metadataHash length mismatch');
).to.be.revertedWith('Asset: 1-Array mismatch');
});
});
describe('Mint Events', function () {
Expand Down Expand Up @@ -761,7 +761,7 @@ describe('Base Asset Contract (/packages/asset/contracts/Asset.sol)', function (
5,
'0x'
)
).to.be.revertedWith('ERC1155: caller is not token owner or approved');
).to.be.revertedWith('Asset: Transfer error');
});
it('should not allow non-owner to transfer a batch of tokens if not approved', async function () {
const {AssetContractAsOwner, mintBatch, minter} = await runAssetSetup();
Expand Down Expand Up @@ -1067,11 +1067,11 @@ describe('Base Asset Contract (/packages/asset/contracts/Asset.sol)', function (

await expect(
Asset.connect(assetAdmin).setOperatorRegistry(zeroAddress)
).to.be.revertedWith("Asset: registry can't be zero address");
).to.be.revertedWith('Asset: Zero address');

await expect(
Asset.connect(assetAdmin).registerAndSubscribe(zeroAddress, true)
).to.be.revertedWith("Asset: subscription can't be zero address");
).to.be.revertedWith('Asset: Zero address');
});

it('should revert when registry is set and subscription is set by non-admin', async function () {
Expand Down
4 changes: 2 additions & 2 deletions packages/asset/test/AssetCreate.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -572,7 +572,7 @@ describe('AssetCreate (/packages/asset/contracts/AssetCreate.sol)', function ()
);
await expect(
mintSingleAsset(signature2, 4, 2, true, metadataHashes[0])
).to.be.revertedWith('Asset: not allowed to reuse metadata hash');
).to.be.revertedWith('Asset: Hash already used');
});
it('should NOT mint same token ids', async function () {
const {
Expand Down Expand Up @@ -1142,7 +1142,7 @@ describe('AssetCreate (/packages/asset/contracts/AssetCreate.sol)', function ()
[true, true],
metadataHashes
)
).to.be.revertedWith('Asset: not allowed to reuse metadata hash');
).to.be.revertedWith('Asset: Hash already used');
});
});
describe('Event', function () {
Expand Down
Loading

1 comment on commit 9eb8a70

@github-actions
Copy link

Choose a reason for hiding this comment

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

Coverage for this commit

96.15%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
packages/asset/contracts
   Asset.sol94.53%89.58%96.43%98.08%116, 207, 309, 309–310, 76
   AssetCreate.sol94.51%83.33%100%100%138, 140, 173, 293, 72
   AssetReveal.sol94.35%86.21%96.55%98.89%143, 147, 181, 376, 416, 424, 441, 75, 98
   AuthSuperValidator.sol90%83.33%100%88.89%51–52
   Catalyst.sol95%91.94%95.45%98.21%125, 127, 140, 152, 224, 80
packages/asset/contracts/interfaces
   IAsset.sol100%100%100%100%
   IAssetCreate.sol100%100%100%100%
   IAssetReveal.sol100%100%100%100%
   ICatalyst.sol100%100%100%100%
   ITokenUtils.sol100%100%100%100%
packages/asset/contracts/libraries
   TokenIdUtils.sol100%100%100%100%
packages/dependency-metatx/contracts
   ERC2771Handler.sol100%100%100%100%
   ERC2771HandlerAbstract.sol100%100%100%100%
   ERC2771HandlerUpgradeable.sol95.45%83.33%100%100%43
packages/dependency-metatx/contracts/test
   ERC2771HandlerTest.sol100%100%100%100%
   ERC2771HandlerUpgradeableTest.sol100%100%100%100%
   MockTrustedForwarder.sol0%0%0%0%15, 18–19, 19, 19–20
packages/dependency-operator-filter/contracts
   OperatorFiltererUpgradeable.sol82.35%85%71.43%83.33%17, 51–52, 61–62, 71, 87
   OperatorFilterSubscription.sol60%50%100%50%19–20
packages/dependency-operator-filter/contracts/interfaces
   IOperatorFilterRegistry.sol100%100%100%100%
packages/dependency-royalty-management/contracts
   MultiRoyaltyDistributor.sol84.06%65%90%92.31%104, 144, 25, 41, 41, 41, 41, 60–61, 97
   RoyaltyDistributor.sol78.95%50%80%90%20, 50, 56
   RoyaltyManager.sol96.39%87.50%100%100%104, 47, 98
   RoyaltySplitter.sol92.08%75%92.31%97.06%117, 157, 185, 218, 65, 72, 83
packages/dependency-royalty-management/contracts/interfaces
   IERC20Approve.sol100%100%100%100%
   IMultiRoyaltyDistributor.sol100%100%100%100%
   IMultiRoyaltyRecipients.sol100%100%100%100%
   IRoyaltyManager.sol100%100%100%100%
   IRoyaltyUGC.sol100%100%100%100%

Please sign in to comment.