From a8e509baf7fba0ead467ad438b220f0b412f1886 Mon Sep 17 00:00:00 2001 From: r4bbit <445106+0x-r4bbit@users.noreply.github.com> Date: Tue, 20 Feb 2024 16:48:58 +0100 Subject: [PATCH] feat(CollectibleV1): add `safeBatchTransferFrom` capabilities This is to allow batch transfers of community collectibles as discussed in #41. Closes #41, #42, #43, #44 --- .gas-snapshot | 81 ++++++++++------- PROPERTIES.md | 21 ++--- README.md | 1 + certora/specs/CollectibleV1.spec | 55 ++++++++++-- contracts/tokens/BaseToken.sol | 38 +++++++- test/CollectibleV1.t.sol | 148 +++++++++++++++++++++++++++++++ 6 files changed, 294 insertions(+), 50 deletions(-) diff --git a/.gas-snapshot b/.gas-snapshot index 9a7105f..cb7f201 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -2,12 +2,13 @@ AddEntryTest:test_AddEntry() (gas: 44392) AddEntryTest:test_RevertWhen_EntryAlreadyExists() (gas: 42644) AddEntryTest:test_RevertWhen_InvalidAddress() (gas: 25133) AddEntryTest:test_RevertWhen_SenderIsNotTokenDeployer() (gas: 14827) -CollectibleV1Test:test_Deployment() (gas: 36386) CommunityERC20Test:test_Deployment() (gas: 35198) CommunityTokenDeployerTest:test_Deployment() (gas: 14805) +CommunityVaultBaseERC20Test:test_Deployment() (gas: 10436) +CommunityVaultBaseERC721Test:test_Deployment() (gas: 10436) CommunityVaultTest:test_Deployment() (gas: 10436) -CreateTest:test_Create() (gas: 2269916) -CreateTest:test_Create() (gas: 2568994) +CreateTest:test_Create() (gas: 2440150) +CreateTest:test_Create() (gas: 2731735) CreateTest:test_RevertWhen_InvalidOwnerTokenAddress() (gas: 15523) CreateTest:test_RevertWhen_InvalidReceiverAddress() (gas: 15656) CreateTest:test_RevertWhen_InvalidSignerPublicKey() (gas: 17057) @@ -17,34 +18,39 @@ CreateTest:test_RevertWhen_SenderIsNotTokenDeployer() (gas: 16421) CreateTest:test_RevertWhen_SenderIsNotTokenDeployer() (gas: 16524) DeployContracts:test() (gas: 120) DeployOwnerAndMasterToken:test() (gas: 120) -DeployTest:test_Deploy() (gas: 4911563) +DeployTest:test_Deploy() (gas: 5244498) DeployTest:test_Deployment() (gas: 14947) -DeployTest:test_RevertWhen_AlreadyDeployed() (gas: 4907793) +DeployTest:test_RevertWhen_AlreadyDeployed() (gas: 5240774) DeployTest:test_RevertWhen_InvalidCommunityAddress() (gas: 51385) DeployTest:test_RevertWhen_InvalidDeployerAddress() (gas: 55272) DeployTest:test_RevertWhen_InvalidDeploymentSignature() (gas: 65617) DeployTest:test_RevertWhen_InvalidSignerPublicKey() (gas: 53433) -DeployTest:test_RevertWhen_InvalidTokenMetadata() (gas: 2694728) +DeployTest:test_RevertWhen_InvalidTokenMetadata() (gas: 2857514) DeploymentTest:test_Deployment() (gas: 14671) DeploymentTest:test_Deployment() (gas: 14671) DeploymentTest:test_Deployment() (gas: 17295) +DeploymentTest:test_Deployment() (gas: 36430) GetEntryTest:test_ReturnZeroAddressIfEntryDoesNotExist() (gas: 11906) MintToTest:test_Deployment() (gas: 35220) -MintToTest:test_Deployment() (gas: 36386) -MintToTest:test_Deployment() (gas: 83220) -MintToTest:test_MintTo() (gas: 178063) -MintToTest:test_MintTo() (gas: 526242) -MintToTest:test_RevertWhen_AddressesAndAmountsAreNotEqualLength() (gas: 29673) -MintToTest:test_RevertWhen_MaxSupplyIsReached() (gas: 20653) -MintToTest:test_RevertWhen_MaxSupplyIsReached() (gas: 511039) -MintToTest:test_RevertWhen_MaxSupplyReached() (gas: 134799) -MintToTest:test_RevertWhen_SenderIsNotOwner() (gas: 31544) -OwnerTokenTest:test_Deployment() (gas: 83220) -RemoteBurnTest:test_Deployment() (gas: 36386) -RemoteBurnTest:test_Deployment() (gas: 83242) -RemoteBurnTest:test_RemoteBurn() (gas: 459164) -RemoteBurnTest:test_RevertWhen_RemoteBurn() (gas: 14768) -RemoteBurnTest:test_RevertWhen_SenderIsNotOwner() (gas: 20379) +MintToTest:test_Deployment() (gas: 83308) +MintToTest:test_MintTo() (gas: 178018) +MintToTest:test_MintTo() (gas: 525865) +MintToTest:test_RevertWhen_AddressesAndAmountsAreNotEqualLength() (gas: 29628) +MintToTest:test_RevertWhen_MaxSupplyIsReached() (gas: 20680) +MintToTest:test_RevertWhen_MaxSupplyIsReached() (gas: 511109) +MintToTest:test_RevertWhen_MaxSupplyReached() (gas: 134754) +MintToTest:test_RevertWhen_SenderIsNotOwner() (gas: 31454) +NotTransferableTest:test_RevertWhen_TokenIsNotTransferable() (gas: 508323) +OwnerTokenTest:test_Deployment() (gas: 83308) +RemoteBurnTest:test_Deployment() (gas: 83330) +RemoteBurnTest:test_RemoteBurn() (gas: 459101) +RemoteBurnTest:test_RevertWhen_RemoteBurn() (gas: 14745) +RemoteBurnTest:test_RevertWhen_SenderIsNotOwner() (gas: 20311) +SafeBatchTransferFromTest:test_RevertWhen_NotAuthorized() (gas: 515628) +SafeBatchTransferFromTest:test_RevertWhen_ReceiverAddressIsZero() (gas: 507780) +SafeBatchTransferFromTest:test_RevertWhen_ReceiversAndIdsMismatch() (gas: 506746) +SafeBatchTransferFromTest:test_SafeBatchTransferFrom() (gas: 435412) +SafeBatchTransferFromTest:test_SafeBatchTransferFromToSingleReceiver() (gas: 422672) SetCommunityTokenDeployerAddressTest:test_RevertWhen_InvalidTokenDeployerAddress() (gas: 12941) SetCommunityTokenDeployerAddressTest:test_RevertWhen_SenderIsNotOwner() (gas: 12482) SetCommunityTokenDeployerAddressTest:test_SetCommunityTokenDeployerAddress() (gas: 22808) @@ -57,22 +63,35 @@ SetMasterTokenFactoryAddressTest:test_RevertWhen_InvalidTokenFactoryAddress() (g SetMasterTokenFactoryAddressTest:test_RevertWhen_SenderIsNotOwner() (gas: 12465) SetMasterTokenFactoryAddressTest:test_SetOwnerTokenFactoryAddress() (gas: 22861) SetMaxSupplyTest:test_Deployment() (gas: 35198) -SetMaxSupplyTest:test_Deployment() (gas: 83242) -SetMaxSupplyTest:test_RevertWhen_CalledBecauseMaxSupplyIsLocked() (gas: 14327) -SetMaxSupplyTest:test_RevertWhen_MaxSupplyLowerThanTotalSupply() (gas: 163641) -SetMaxSupplyTest:test_RevertWhen_SenderIsNotOwner() (gas: 12527) -SetMaxSupplyTest:test_RevertWhen_SenderIsNotOwner() (gas: 21445) -SetMaxSupplyTest:test_SetMaxSupply() (gas: 23998) +SetMaxSupplyTest:test_Deployment() (gas: 83330) +SetMaxSupplyTest:test_RevertWhen_CalledBecauseMaxSupplyIsLocked() (gas: 14304) +SetMaxSupplyTest:test_RevertWhen_MaxSupplyLowerThanTotalSupply() (gas: 163551) +SetMaxSupplyTest:test_RevertWhen_SenderIsNotOwner() (gas: 12459) +SetMaxSupplyTest:test_RevertWhen_SenderIsNotOwner() (gas: 21355) +SetMaxSupplyTest:test_SetMaxSupply() (gas: 23953) SetOwnerTokenFactoryAddressTest:test_Deployment() (gas: 14805) SetOwnerTokenFactoryAddressTest:test_RevertWhen_InvalidTokenFactoryAddress() (gas: 12970) SetOwnerTokenFactoryAddressTest:test_RevertWhen_SenderIsNotOwner() (gas: 12443) SetOwnerTokenFactoryAddressTest:test_SetOwnerTokenFactoryAddress() (gas: 22840) -SetSignerPublicKeyTest:test_Deployment() (gas: 83220) -SetSignerPublicKeyTest:test_RevertWhen_SenderIsNotOwner() (gas: 13222) -SetSignerPublicKeyTest:test_SetSignerPublicKey() (gas: 24163) +SetSignerPublicKeyTest:test_Deployment() (gas: 83308) +SetSignerPublicKeyTest:test_RevertWhen_SenderIsNotOwner() (gas: 13154) +SetSignerPublicKeyTest:test_SetSignerPublicKey() (gas: 24162) SetTokenDeployerAddressTest:test_RevertWhen_InvalidTokenDeployerAddress() (gas: 12964) SetTokenDeployerAddressTest:test_RevertWhen_InvalidTokenDeployerAddress() (gas: 12964) SetTokenDeployerAddressTest:test_RevertWhen_SenderIsNotOwner() (gas: 12438) SetTokenDeployerAddressTest:test_RevertWhen_SenderIsNotOwner() (gas: 12438) SetTokenDeployerAddressTest:test_SetTokenDeployerAddress() (gas: 22768) -SetTokenDeployerAddressTest:test_SetTokenDeployerAddress() (gas: 22768) \ No newline at end of file +SetTokenDeployerAddressTest:test_SetTokenDeployerAddress() (gas: 22768) +TransferERC20ByAdminTest:test_AdminCanTransferERC20() (gas: 84406) +TransferERC20ByAdminTest:test_Deployment() (gas: 10556) +TransferERC20ByAdminTest:test_LengthMismatch() (gas: 31872) +TransferERC20ByAdminTest:test_NoRecipients() (gas: 25198) +TransferERC20ByAdminTest:test_TransferAmountZero() (gas: 61686) +TransferERC20ByNonAdminTest:test_Deployment() (gas: 10458) +TransferERC20ByNonAdminTest:test_revertIfCalledByNonAdmin() (gas: 35570) +TransferERC721ByAdminTest:test_AdminCanTransferERC721() (gas: 107114) +TransferERC721ByAdminTest:test_Deployment() (gas: 10556) +TransferERC721ByAdminTest:test_LengthMismatch() (gas: 31875) +TransferERC721ByAdminTest:test_NoRecipients() (gas: 25213) +TransferERC721ByNonAdminTest:test_Deployment() (gas: 10458) +TransferERC721ByNonAdminTest:test_RevertIfCalledByNonAdmin() (gas: 35563) \ No newline at end of file diff --git a/PROPERTIES.md b/PROPERTIES.md index 1356a6d..59a7de8 100644 --- a/PROPERTIES.md +++ b/PROPERTIES.md @@ -8,13 +8,14 @@ Below is a list of all documented properties and invariants of this project that - **Risk** - One of **High**, **Medium** and **Low**, depending on the property's risk factor - **Tested** - Whether this property has been (fuzz) tested -| **Property** | **Type** | **Risk** | **Tested** | -| ------------------------------------------------------------------------- | ------------------- | -------- | ---------- | -| Only allows deployment with valid signature | Unit test | High | Yes | -| Adds Owner token entry to registry upon deployment | Unit test | Low | Yes | -| Only one deployment per account allowed | Unit test | Medium | Yes | -| One and only one owner token address exists in the registry per community | Valid state | High | Yes | -| If deployment registry address changes, sender must be owner | Variable transition | High | Yes | -| If owner token factory address changes, sender must be owner | Variable transition | High | Yes | -| If master token factory address changes, sender must be owner | Variable transition | High | Yes | -| Registry grows as the more accounts perform a deployment | High-Level Property | Low | No | +| **Property** | **Type** | **Risk** | **Tested** | +| --------------------------------------------------------------------------------------------------------- | ------------------- | -------- | ---------- | +| Only allows deployment with valid signature | Unit test | High | Yes | +| Adds Owner token entry to registry upon deployment | Unit test | Low | Yes | +| Only one deployment per account allowed | Unit test | Medium | Yes | +| One and only one owner token address exists in the registry per community | Valid state | High | Yes | +| If deployment registry address changes, sender must be owner | Variable transition | High | Yes | +| If owner token factory address changes, sender must be owner | Variable transition | High | Yes | +| If master token factory address changes, sender must be owner | Variable transition | High | Yes | +| Registry grows as the more accounts perform a deployment | High-Level Property | Low | No | +| No addresses other than the receiver addresses change an addresse's balance when batch transfering tokens | High-Level Property | High | Yes | diff --git a/README.md b/README.md index afa0c1e..4cf52a8 100644 --- a/README.md +++ b/README.md @@ -162,6 +162,7 @@ Below is a description of all community tokens that can be deployed and minted t - The ability to configure a maximum supply. This is used for both `MasterToken` and `CollectibleV1` tokens. - A `mintTo` function that allows for minting tokens to multiple addresses at once. - A mechanism to burn tokens "remotely". The use case here is to remove token masters or admins privileges. +- The ability to batch transfer tokens to multiple receivers. Not all inheriting contracts make use of all of the custom functionality. diff --git a/certora/specs/CollectibleV1.spec b/certora/specs/CollectibleV1.spec index 986182b..e2df7d2 100644 --- a/certora/specs/CollectibleV1.spec +++ b/certora/specs/CollectibleV1.spec @@ -7,6 +7,7 @@ methods { function setMaxSupply(uint256 newMaxSupply) external returns (uint); function mintTo(address[]) external; function mintedCount() external returns (uint) envfree; + function safeBatchTransferFrom(address from, address[] to, uint256[] tokenIds, bytes data) external; function countAddressOccurrences(address[], address) external returns (uint) envfree; function _.onERC721Received(address, address, uint256, bytes) external => NONDET; } @@ -128,12 +129,52 @@ rule mintToRelations() { assert balance_s1 == balance_s2; } -rule shouldPass { - assert true; +rule safeBatchTransferFromReverts() { + + address from; + address[] to; + uint256[] tokenIds; + bytes data; + env e; + + mathint supply_before = totalSupply(); + mathint max_supply = maxSupply(); + mathint minted_count = mintedCount(); + + require supply_before <= max_supply; + require minted_count >= supply_before; + + safeBatchTransferFrom@withrevert(e, from, to, tokenIds, data); + + bool reverted = lastReverted; + + assert (tokenIds.length != to.length) => reverted; } -/* rule sanity(env e, method f) { */ -/* calldataarg args; */ -/* f(e, args); */ -/* assert false; */ -/* } */ +rule safeBatchTransferFromRelations() { + address from; + address[] to; + uint256[] tokenIds; + bytes data; + env e; + + require to.length == tokenIds.length; + + mathint supply_before = totalSupply(); + mathint max_supply = maxSupply(); + mathint minted_count = mintedCount(); + + require supply_before <= max_supply; + require minted_count >= supply_before; + + address a; + + storage s1 = lastStorage; + safeBatchTransferFrom(e, from, to, tokenIds, data); + mathint balance_s1 = balanceOf(a); + + safeBatchTransferFrom(e, from, to, tokenIds, data) at s1; + + mathint balance_s2 = balanceOf(a); + assert balance_s1 == balance_s2; +} diff --git a/contracts/tokens/BaseToken.sol b/contracts/tokens/BaseToken.sol index af46621..6aad077 100644 --- a/contracts/tokens/BaseToken.sol +++ b/contracts/tokens/BaseToken.sol @@ -15,6 +15,8 @@ abstract contract BaseToken is Context, ERC721Enumerable, CommunityOwnable { error BaseToken_MaxSupplyReached(); error BaseToken_NotRemoteBurnable(); error BaseToken_NotTransferable(); + error BaseToken_NotAuthorized(); + error BaseToken_ReceiversAndIdsMismatch(); /// @notice Emits a custom mint event for Status applications to listen to /// @dev This is doubling the {Transfer} event from ERC721 but we need to emit this @@ -134,8 +136,8 @@ abstract contract BaseToken is Context, ERC721Enumerable, CommunityOwnable { } /** - * @notice - * @dev + * @notice Overrides `ERC721Enumerable` to add a check for transferability + * @dev See {ERC721-_beforeTokenTransfer}. */ function _beforeTokenTransfer( address from, @@ -153,5 +155,37 @@ abstract contract BaseToken is Context, ERC721Enumerable, CommunityOwnable { super._beforeTokenTransfer(from, to, firstTokenId, batchSize); } + /** + * @notice Batch transfers tokens by passing an array of token IDs. + * @dev This is simply delegates to `safeTranferFrom` for each token ID in the array. + */ + function safeBatchTransferFrom( + address from, + address[] calldata receivers, + uint256[] calldata ids, + bytes memory data + ) + public + virtual + { + _safeBatchTransferFrom(from, receivers, ids, data); + } + + function _safeBatchTransferFrom( + address from, + address[] memory receivers, + uint256[] calldata ids, + bytes memory data + ) + internal + { + if (receivers.length != ids.length) { + revert BaseToken_ReceiversAndIdsMismatch(); + } + + for (uint256 i = 0; i < ids.length; ++i) { + safeTransferFrom(from, receivers[i], ids[i], data); + } + } // Private functions } diff --git a/test/CollectibleV1.t.sol b/test/CollectibleV1.t.sol index a50d19b..dc95f44 100644 --- a/test/CollectibleV1.t.sol +++ b/test/CollectibleV1.t.sol @@ -37,7 +37,9 @@ contract CollectibleV1Test is Test { accounts[2] = makeAddr("three"); accounts[3] = makeAddr("four"); } +} +contract DeploymentTest is CollectibleV1Test { function test_Deployment() public { assertEq(collectibleV1.name(), name); assertEq(collectibleV1.symbol(), symbol); @@ -116,3 +118,149 @@ contract RemoteBurnTest is CollectibleV1Test { assertEq(collectibleV1.totalSupply(), maxSupply - 1); } } + +contract SafeBatchTransferFromTest is CollectibleV1Test { + function setUp() public virtual override { + CollectibleV1Test.setUp(); + } + + function test_RevertWhen_ReceiversAndIdsMismatch() public { + // ensure sender owns a token + vm.prank(deployer); + collectibleV1.mintTo(accounts); + + address[] memory receivers = new address[](1); + receivers[0] = accounts[1]; + + // ids must be of same length as `receivers` + uint256[] memory ids = new uint256[](2); + ids[0] = 0; + ids[1] = 1; + + vm.prank(accounts[0]); + vm.expectRevert(BaseToken.BaseToken_ReceiversAndIdsMismatch.selector); + collectibleV1.safeBatchTransferFrom(accounts[0], receivers, ids, ""); + } + + function test_RevertWhen_NotAuthorized() public { + vm.prank(deployer); + collectibleV1.mintTo(accounts); + + address[] memory receivers = new address[](1); + receivers[0] = accounts[3]; + + // ids must be of same length as `accounts` + uint256[] memory ids = new uint256[](1); + ids[0] = 0; + + // ensure accounts[3] has no ownership or approval of token with id `0` + assertEq(collectibleV1.ownerOf(ids[0]), accounts[0]); + assertEq(collectibleV1.isApprovedForAll(accounts[0], receivers[0]), false); + + vm.prank(receivers[0]); + vm.expectRevert(bytes("ERC721: caller is not token owner or approved")); + collectibleV1.safeBatchTransferFrom(accounts[0], receivers, ids, ""); + } + + function test_RevertWhen_ReceiverAddressIsZero() public { + // ensure sender owns a token + vm.prank(deployer); + collectibleV1.mintTo(accounts); + + address[] memory receivers = new address[](1); + receivers[0] = address(0); + + uint256[] memory ids = new uint256[](1); + ids[0] = 0; + + vm.prank(accounts[0]); + vm.expectRevert(bytes("ERC721: transfer to the zero address")); + collectibleV1.safeBatchTransferFrom(accounts[0], receivers, ids, ""); + } + + function test_SafeBatchTransferFrom() public { + // ensure sender owns a token + vm.prank(deployer); + address[] memory sameAddresses = new address[](3); + sameAddresses[0] = accounts[0]; + sameAddresses[1] = accounts[0]; + sameAddresses[2] = accounts[0]; + collectibleV1.mintTo(sameAddresses); + + address[] memory receivers = new address[](3); + receivers[0] = accounts[1]; + receivers[1] = accounts[2]; + receivers[2] = accounts[3]; + + uint256[] memory ids = new uint256[](3); + ids[0] = 0; + ids[1] = 1; + ids[2] = 2; + + vm.prank(accounts[0]); + collectibleV1.safeBatchTransferFrom(accounts[0], receivers, ids, ""); + + assertEq(collectibleV1.balanceOf(accounts[0]), 0); + assertEq(collectibleV1.balanceOf(accounts[1]), 1); + assertEq(collectibleV1.balanceOf(accounts[2]), 1); + assertEq(collectibleV1.balanceOf(accounts[3]), 1); + } + + function test_SafeBatchTransferFromToSingleReceiver() public { + // ensure sender owns a token + vm.prank(deployer); + address[] memory sameAddresses = new address[](3); + sameAddresses[0] = accounts[0]; + sameAddresses[1] = accounts[0]; + sameAddresses[2] = accounts[0]; + collectibleV1.mintTo(sameAddresses); + + address[] memory receivers = new address[](3); + receivers[0] = accounts[1]; + receivers[1] = accounts[1]; + receivers[2] = accounts[1]; + + uint256[] memory ids = new uint256[](3); + ids[0] = 0; + ids[1] = 1; + ids[2] = 2; + + vm.prank(accounts[0]); + collectibleV1.safeBatchTransferFrom(accounts[0], receivers, ids, ""); + + assertEq(collectibleV1.balanceOf(accounts[0]), 0); + assertEq(collectibleV1.balanceOf(accounts[1]), 3); + } +} + +contract NotTransferableTest is CollectibleV1Test { + function setUp() public virtual override { + DeployOwnerAndMasterToken deployment = new DeployOwnerAndMasterToken(); + (OwnerToken ownerToken, MasterToken masterToken, DeploymentConfig deploymentConfig) = deployment.run(); + deployer = deploymentConfig.deployer(); + + collectibleV1 = new CollectibleV1( + name, symbol, maxSupply, remoteBurnable, false, baseURI, address(ownerToken), address(masterToken) + ); + + accounts[0] = makeAddr("one"); + accounts[1] = makeAddr("two"); + accounts[2] = makeAddr("three"); + accounts[3] = makeAddr("four"); + } + + function test_RevertWhen_TokenIsNotTransferable() public { + // ensure accounts own tokens + vm.prank(deployer); + collectibleV1.mintTo(accounts); + uint256[] memory ids = new uint256[](4); + ids[0] = 0; + ids[1] = 1; + ids[2] = 2; + ids[3] = 3; + + vm.prank(accounts[0]); + vm.expectRevert(BaseToken.BaseToken_NotTransferable.selector); + collectibleV1.safeBatchTransferFrom(accounts[0], accounts, ids, ""); + } +}