From d05f7723d79124b1c84a9e7a3e799416701df9c0 Mon Sep 17 00:00:00 2001 From: george-openformat Date: Wed, 25 Sep 2024 15:00:08 +0100 Subject: [PATCH 1/4] feat: add operator check --- src/facet/RewardsFacet.sol | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/facet/RewardsFacet.sol b/src/facet/RewardsFacet.sol index 6beacfa..ac6ddf6 100644 --- a/src/facet/RewardsFacet.sol +++ b/src/facet/RewardsFacet.sol @@ -52,6 +52,10 @@ contract RewardsFacet is Multicall, SafeOwnable { bytes32 _activityType, string calldata _uri ) public { + if (msg.sender != _operator()) { + revert RewardsFacet_NotAuthorized(); + } + if (!_canMint(_token)) { revert RewardsFacet_NotAuthorized(); } @@ -68,6 +72,10 @@ contract RewardsFacet is Multicall, SafeOwnable { bytes32 _activityType, string calldata _uri ) public { + if (msg.sender != _operator()) { + revert RewardsFacet_NotAuthorized(); + } + Token(_token).transferFrom(msg.sender, _to, _amount); emit TokenTransferred(_token, _to, _amount, _id, _activityType, _uri); } @@ -92,6 +100,10 @@ contract RewardsFacet is Multicall, SafeOwnable { bytes32 _activityType, bytes calldata _data ) public { + if (msg.sender != _operator()) { + revert RewardsFacet_NotAuthorized(); + } + if (!_canMint(_badgeContract)) { revert RewardsFacet_NotAuthorized(); } @@ -118,6 +130,10 @@ contract RewardsFacet is Multicall, SafeOwnable { bytes32 _activityType, bytes calldata _data ) public { + if (msg.sender != _operator()) { + revert RewardsFacet_NotAuthorized(); + } + if (!_canMint(_badgeContract)) { revert RewardsFacet_NotAuthorized(); } @@ -135,6 +151,10 @@ contract RewardsFacet is Multicall, SafeOwnable { bytes32 _activityType, string calldata _uri ) public { + if (msg.sender != _operator()) { + revert RewardsFacet_NotAuthorized(); + } + if (!_canMint(_token)) { revert RewardsFacet_NotAuthorized(); } @@ -150,10 +170,18 @@ contract RewardsFacet is Multicall, SafeOwnable { bytes32 _activityType, string calldata _uri ) public { + if (msg.sender != _operator()) { + revert RewardsFacet_NotAuthorized(); + } + NFT(_token).transferFrom(msg.sender, _to, _tokenId); emit BadgeTransferred(_token, _to, _tokenId, _id, _activityType, _uri); } + function _operator() internal virtual returns (address) { + return _owner(); + } + function _canMint(address _token) internal virtual returns (bool) { return Token(_token).hasRole(ADMIN_ROLE, msg.sender) || Token(_token).hasRole(MINTER_ROLE, msg.sender); } From bb70a40c15e2f7fe858cdf31d14d462e01e41ae0 Mon Sep 17 00:00:00 2001 From: george-openformat Date: Wed, 25 Sep 2024 15:20:38 +0100 Subject: [PATCH 2/4] test: add caller is not app operator tests --- test/integration/facet/RewardsFacet.t.sol | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/test/integration/facet/RewardsFacet.t.sol b/test/integration/facet/RewardsFacet.t.sol index 00792ae..a9970c8 100644 --- a/test/integration/facet/RewardsFacet.t.sol +++ b/test/integration/facet/RewardsFacet.t.sol @@ -85,6 +85,7 @@ contract Setup is Test, Helpers { // ipfs uri taken from https://docs.ipfs.tech/how-to/best-practices-for-nft-data/#types-of-ipfs-links-and-when-to-use-them string baseURI = "ipfs://bafybeibnsoufr2renqzsh347nrx54wcubt5lgkeivez63xvivplfwhtpym/"; uint16 tenPercentBPS = 1000; + uint256 oneEther = 1 ether; event ERC721Minted(address token, uint256 quantity, address to, bytes32 id, bytes32 activityType, string uri); event BadgeMinted( @@ -97,7 +98,7 @@ contract Setup is Test, Helpers { other = address(0x11); socialConscious = address(0x12); - vm.deal(creator, 1 ether); + vm.deal(creator, oneEther); // deploy contracts globals = new Globals(); @@ -223,6 +224,15 @@ contract RewardFacet__integration_mintBadge is Setup { vm.prank(creator); RewardsFacet(address(app)).mintBadge(badgeContract, other, activityId, activityType, abi.encode("testing 123")); } + + function test_reverts_when_caller_is_not_the_app_operator() public { + vm.prank(creator); + ERC721Badge(badgeContract).grantRole(MINTER_ROLE, other); + + vm.prank(other); + vm.expectRevert(); + RewardsFacet(address(app)).mintBadge(badgeContract, other, activityId, activityType, ""); + } } contract RewardFacet__integration_batchMintBadge is Setup { @@ -266,6 +276,15 @@ contract RewardFacet__integration_batchMintBadge is Setup { RewardsFacet(address(app)).batchMintBadge(badgeContract, other, 10, activityId, activityType, ""); vm.stopPrank(); } + + function test_reverts_when_caller_is_not_the_app_operator() public { + vm.prank(creator); + ERC721Badge(badgeContract).grantRole(MINTER_ROLE, other); + + vm.prank(other); + vm.expectRevert(); + RewardsFacet(address(app)).batchMintBadge(badgeContract, other, 10, activityId, activityType, ""); + } } contract RewardFacet__integration_multicall is Setup { From 073f4419a238aa3254b914149a7ce6af17d19560 Mon Sep 17 00:00:00 2001 From: george-openformat Date: Wed, 25 Sep 2024 15:21:45 +0100 Subject: [PATCH 3/4] test: add tests for mintERC20 and transferERC20 --- test/integration/facet/RewardsFacet.t.sol | 185 ++++++++++++++++++---- 1 file changed, 157 insertions(+), 28 deletions(-) diff --git a/test/integration/facet/RewardsFacet.t.sol b/test/integration/facet/RewardsFacet.t.sol index a9970c8..0ef8ce8 100644 --- a/test/integration/facet/RewardsFacet.t.sol +++ b/test/integration/facet/RewardsFacet.t.sol @@ -304,44 +304,173 @@ contract RewardFacet__integration_multicall is Setup { } } -contract ERC20Base_Setup is Setup { - ERC20Base base; - MinterDummy minter; +contract RewardsFacet__integration_mintERC20 is Setup { + event TokenMinted(address token, address to, uint256 amount, bytes32 id, bytes32 activityType, string uri); + address tokenContract; function _afterSetup() internal override { - // add lazy mint implementation - ERC20Base baseImplementation = new ERC20Base(); - bytes32 baseImplementationId = bytes32("Base"); - globals.setERC20Implementation(baseImplementationId, address(baseImplementation)); + // use app to create credit contract + vm.prank(creator); + tokenContract = ERC20FactoryFacet(address(app)).createERC20( + name, symbol, 18, oneEther, erc20ImplementationId + ); + } - // create lazy mint erc20 + function test_setup() public { + assertEq(ERC20Base(tokenContract).balanceOf(creator), oneEther); + } + + function test_mints_erc20() public { vm.prank(creator); - // forgefmt: disable-start - base = ERC20Base( - ERC20FactoryFacet(address(app)).createERC20( - "name", - "symbol", - 18, - 0, - baseImplementationId - ) + RewardsFacet(address(app)).mintERC20( + tokenContract, + other, + oneEther, + activityId, + activityType, + "" + ); + + assertEq(ERC20Base(tokenContract).balanceOf(other), oneEther); + } + + function test_emits_token_minted_event() public { + vm.expectEmit(true, true, true, true, address(app)); + emit TokenMinted( + tokenContract, + other, + oneEther, + activityId, + activityType, + "" ); - // forgefmt: disable-end - // create contract that can mint - minter = new MinterDummy(); - // grant minter role to minter contract vm.prank(creator); - base.grantRole(MINTER_ROLE, address(minter)); + RewardsFacet(address(app)).mintERC20( + tokenContract, + other, + oneEther, + activityId, + activityType, + "" + ); } -} -contract RewardsFacet__integration_mintERC20 is ERC20Base_Setup { - function test_mints_erc20() public { + function test_reverts_when_caller_is_not_the_app_operator() public { + // other has minter role vm.prank(creator); - base.mintTo(creator, 1000); + ERC20Base(tokenContract).grantRole(MINTER_ROLE, other); - // check nft is minted to creator - assertEq(base.balanceOf(creator), 1000); + vm.prank(other); + vm.expectRevert(); + RewardsFacet(address(app)).mintERC20( + tokenContract, + other, + oneEther, + activityId, + activityType, + "" + ); + } + + function test_reverts_when_caller_does_not_have_correct_roles_on_token_contract() public { + // revoke admin and minter roles + vm.prank(creator); + ERC20Base(tokenContract).revokeRole(MINTER_ROLE, creator); + vm.prank(creator); + ERC20Base(tokenContract).revokeRole(ADMIN_ROLE, creator); + + vm.prank(other); + vm.expectRevert(); + RewardsFacet(address(app)).mintERC20( + tokenContract, + other, + oneEther, + activityId, + activityType, + "" + ); } } + +contract RewardsFacet__integration_transferERC20 is Setup { + event TokenTransferred(address token, address to, uint256 amount, bytes32 id, bytes32 activityType, string uri); + + address tokenContract; + + function _afterSetup() internal override { + // use app to create credit contract + vm.prank(creator); + tokenContract = ERC20FactoryFacet(address(app)).createERC20( + name, + symbol, + 18, + oneEther, + erc20ImplementationId + ); + + // approve spend allowance for app contract + vm.prank(creator); + ERC20Base(tokenContract).approve(address(app), oneEther); + } + + function test_setup () public { + assertEq(ERC20Base(tokenContract).balanceOf(creator), oneEther); + assertEq(ERC20Base(tokenContract).allowance(creator, address(app)), oneEther); + } + + function test_transfers_erc20() public { + vm.prank(creator); + RewardsFacet(address(app)).transferERC20( + tokenContract, + other, + oneEther, + activityId, + activityType, + "" + ); + + assertEq(ERC20Base(tokenContract).balanceOf(other), oneEther); + } + + function test_emits_token_transferred_event() public { + vm.expectEmit(true, true, true, true, address(app)); + emit TokenTransferred( + tokenContract, + other, + oneEther, + activityId, + activityType, + "" + ); + + vm.prank(creator); + RewardsFacet(address(app)).transferERC20( + tokenContract, + other, + oneEther, + activityId, + activityType, + "" + ); + } + + function test_reverts_when_caller_is_not_the_app_operator() public { + // other has a balance and has approved spend on token contract + vm.prank(creator); + ERC20Base(tokenContract).mintTo(other, oneEther); + vm.prank(other); + ERC20Base(tokenContract).approve(address(app), oneEther); + + vm.prank(other); + vm.expectRevert(); + RewardsFacet(address(app)).transferERC20( + tokenContract, + other, + oneEther, + activityId, + activityType, + "" + ); + } +} \ No newline at end of file From 534de5a85f7ed4ab475c15bdd3ae810761925ba9 Mon Sep 17 00:00:00 2001 From: george-openformat Date: Wed, 25 Sep 2024 17:07:48 +0100 Subject: [PATCH 4/4] test: add tests for mintERC721 and transferERC721 --- test/integration/facet/RewardsFacet.t.sol | 180 ++++++++++++++++++---- 1 file changed, 151 insertions(+), 29 deletions(-) diff --git a/test/integration/facet/RewardsFacet.t.sol b/test/integration/facet/RewardsFacet.t.sol index 0ef8ce8..03b9c61 100644 --- a/test/integration/facet/RewardsFacet.t.sol +++ b/test/integration/facet/RewardsFacet.t.sol @@ -18,6 +18,7 @@ import {RegistryMock} from "src/registry/RegistryMock.sol"; import {AppFactory} from "src/factories/App.sol"; import {Globals} from "src/globals/Globals.sol"; +import {ERC721Base} from "src/tokens/ERC721/ERC721Base.sol"; import {ERC721Badge} from "src/tokens/ERC721/ERC721Badge.sol"; import {IERC721Factory} from "@extensions/ERC721Factory/IERC721Factory.sol"; import {ERC721FactoryFacet} from "src/facet/ERC721FactoryFacet.sol"; @@ -68,16 +69,16 @@ contract Setup is Test, Helpers { SettingsFacet settingsFacet; RewardsFacet rewardsFacet; - ERC721Badge erc721Implementation; + ERC721Base erc721Implementation; bytes32 erc721ImplementationId; + ERC721Badge badgeImplementation; + bytes32 badgeImplementationId; ERC721FactoryFacet erc721FactoryFacet; ERC20Base erc20Implementation; bytes32 erc20ImplementationId; ERC20FactoryFacet erc20FactoryFacet; - bytes32 badgeImplementationId = bytes32("Badge"); - address badgeContract; string name = "Name"; string symbol = "Symbol"; bytes32 activityId = "collected a berry"; @@ -87,10 +88,11 @@ contract Setup is Test, Helpers { uint16 tenPercentBPS = 1000; uint256 oneEther = 1 ether; + event TokenMinted(address token, address to, uint256 amount, bytes32 id, bytes32 activityType, string uri); + event TokenTransferred(address token, address to, uint256 amount, bytes32 id, bytes32 activityType, string uri); event ERC721Minted(address token, uint256 quantity, address to, bytes32 id, bytes32 activityType, string uri); - event BadgeMinted( - address token, uint256 quantity, address to, bytes32 activityId, bytes32 activityType, bytes data - ); + event BadgeMinted(address token, uint256 quantity, address to, bytes32 activityId, bytes32 activityType, bytes data); + event BadgeTransferred(address token, address to, uint256 tokenId, bytes32 id, bytes32 activityType, string uri); function setUp() public { // assign addresses @@ -106,8 +108,10 @@ contract Setup is Test, Helpers { appImplementation = new Proxy(true); appFactory = new AppFactory(address(appImplementation), address(registry), address(globals)); - erc721Implementation = new ERC721Badge(false); - erc721ImplementationId = bytes32("Badge"); + erc721Implementation = new ERC721Base(); + erc721ImplementationId = bytes32("Base"); + badgeImplementation = new ERC721Badge(false); + badgeImplementationId = bytes32("Badge"); erc721FactoryFacet = new ERC721FactoryFacet(); erc20Implementation = new ERC20Base(); @@ -120,6 +124,7 @@ contract Setup is Test, Helpers { // setup globals globals.setPlatformFee(0, 0, socialConscious); + globals.setERC721Implementation(badgeImplementationId, address(badgeImplementation)); globals.setERC721Implementation(erc721ImplementationId, address(erc721Implementation)); globals.setERC20Implementation(erc20ImplementationId, address(erc20Implementation)); @@ -186,12 +191,6 @@ contract Setup is Test, Helpers { ); } - // use app to create badge contract - vm.prank(creator); - badgeContract = ERC721FactoryFacet(address(app)).createERC721WithTokenURI( - name, symbol, baseURI, creator, uint16(tenPercentBPS), badgeImplementationId - ); - _afterSetup(); } @@ -202,6 +201,16 @@ contract Setup is Test, Helpers { } contract RewardFacet__integration_mintBadge is Setup { + address badgeContract; + + function _afterSetup() internal override { + // use app to create badge contract + vm.prank(creator); + badgeContract = ERC721FactoryFacet(address(app)).createERC721WithTokenURI( + name, symbol, baseURI, creator, uint16(tenPercentBPS), badgeImplementationId + ); + } + function test_rewards_badge() public { vm.prank(creator); RewardsFacet(address(app)).mintBadge(badgeContract, other, activityId, activityType, ""); @@ -230,12 +239,22 @@ contract RewardFacet__integration_mintBadge is Setup { ERC721Badge(badgeContract).grantRole(MINTER_ROLE, other); vm.prank(other); - vm.expectRevert(); + vm.expectRevert(RewardsFacet.RewardsFacet_NotAuthorized.selector); RewardsFacet(address(app)).mintBadge(badgeContract, other, activityId, activityType, ""); } } contract RewardFacet__integration_batchMintBadge is Setup { + address badgeContract; + + function _afterSetup() internal override { + // use app to create badge contract + vm.prank(creator); + badgeContract = ERC721FactoryFacet(address(app)).createERC721WithTokenURI( + name, symbol, baseURI, creator, uint16(tenPercentBPS), badgeImplementationId + ); + } + function test_rewards_multiple_badges() public { vm.prank(creator); RewardsFacet(address(app)).batchMintBadge(badgeContract, other, 10, activityId, activityType, ""); @@ -261,33 +280,139 @@ contract RewardFacet__integration_batchMintBadge is Setup { ); } - function test_reverts_when_sender_not_authorised() public { - vm.expectRevert(RewardsFacet.RewardsFacet_NotAuthorized.selector); + function test_reverts_when_app_is_not_granted_minter_role() public { + vm.startPrank(creator); + ERC721Badge(badgeContract).revokeRole(MINTER_ROLE, address(app)); + + vm.expectRevert(ERC721Badge.ERC721Badge_notAuthorized.selector); + RewardsFacet(address(app)).batchMintBadge(badgeContract, other, 10, activityId, activityType, ""); + vm.stopPrank(); + } + + function test_reverts_when_caller_is_not_the_app_operator() public { + vm.prank(creator); + ERC721Badge(badgeContract).grantRole(MINTER_ROLE, other); vm.prank(other); + vm.expectRevert(RewardsFacet.RewardsFacet_NotAuthorized.selector); RewardsFacet(address(app)).batchMintBadge(badgeContract, other, 10, activityId, activityType, ""); } +} + +contract RewardFacet__integration_mintERC721 is Setup { + address baseContract; + + function _afterSetup() internal override { + // use app to create badge contract + vm.prank(creator); + baseContract = ERC721FactoryFacet(address(app)).createERC721( + name, symbol, creator, uint16(tenPercentBPS), erc721ImplementationId + ); + } + + function test_rewards_multiple_badges() public { + vm.prank(creator); + RewardsFacet(address(app)).mintERC721(baseContract, other, 10, baseURI, activityId, activityType, ""); + + assertEq(ERC721Base(baseContract).balanceOf(other), 10); + } + + function test_emits_erc721_minted_event() public { + vm.expectEmit(true, true, true, true); + emit ERC721Minted(baseContract, 10, other, activityId, activityType, ""); + + vm.prank(creator); + RewardsFacet(address(app)).mintERC721(baseContract, other, 10, baseURI, activityId, activityType, ""); + } function test_reverts_when_app_not_granted_minter_role() public { vm.startPrank(creator); - ERC721Badge(badgeContract).revokeRole(MINTER_ROLE, address(app)); + ERC721Base(baseContract).revokeRole(MINTER_ROLE, address(app)); - vm.expectRevert(); - RewardsFacet(address(app)).batchMintBadge(badgeContract, other, 10, activityId, activityType, ""); + vm.expectRevert(ERC721Base.ERC721Base_notAuthorized.selector); + RewardsFacet(address(app)).mintERC721(baseContract, other, 10, baseURI, activityId, activityType, ""); vm.stopPrank(); } function test_reverts_when_caller_is_not_the_app_operator() public { vm.prank(creator); - ERC721Badge(badgeContract).grantRole(MINTER_ROLE, other); + ERC721Base(baseContract).grantRole(MINTER_ROLE, other); vm.prank(other); - vm.expectRevert(); - RewardsFacet(address(app)).batchMintBadge(badgeContract, other, 10, activityId, activityType, ""); + vm.expectRevert(RewardsFacet.RewardsFacet_NotAuthorized.selector); + RewardsFacet(address(app)).mintERC721(baseContract, other, 10, baseURI, activityId, activityType, ""); + } +} + +contract RewardFacet__integration_transferERC721 is Setup { + address baseContract; + + function _afterSetup() internal override { + // use app to create badge contract + vm.startPrank(creator); + baseContract = ERC721FactoryFacet(address(app)).createERC721( + name, symbol, creator, uint16(tenPercentBPS), erc721ImplementationId + ); + + // mint nft to creator and approve app as operator + ERC721Base(baseContract).mintTo(creator, baseURI); + ERC721Base(baseContract).setApprovalForAll(address(app), true); + vm.stopPrank(); + } + + function test_setup() public { + assertEq(ERC721Base(baseContract).ownerOf(0), creator); + assertEq(ERC721Base(baseContract).isApprovedOrOwner(address(app), 0), true); + } + + function test_transfers_erc721() public { + vm.prank(creator); + RewardsFacet(address(app)).transferERC721(baseContract, other, 0, activityId, activityType, ""); + + assertEq(ERC721Base(baseContract).ownerOf(0), other); + assertEq(ERC721Base(baseContract).isApprovedOrOwner(address(app), 0), false); + } + + function test_emits_badge_transferred_event() public { + vm.expectEmit(true, true, true, true); + emit BadgeTransferred(baseContract, other, 0, activityId, activityType, ""); + + vm.prank(creator); + RewardsFacet(address(app)).transferERC721(baseContract, other, 0, activityId, activityType, ""); + } + + function test_reverts_when_app_not_approved() public { + ERC721Base(baseContract).setApprovalForAll(address(app), false); + vm.expectRevert(RewardsFacet.RewardsFacet_NotAuthorized.selector); + + vm.prank(other); + RewardsFacet(address(app)).transferERC721(baseContract, other, 0, activityId, activityType, ""); + } + + function test_reverts_when_caller_is_not_the_app_operator() public { + // mint token 2 to other and approve app + vm.prank(creator); + ERC721Base(baseContract).mintTo(other, baseURI); + vm.prank(other); + ERC721Base(baseContract).setApprovalForAll(address(app), true); + + vm.prank(other); + vm.expectRevert(RewardsFacet.RewardsFacet_NotAuthorized.selector); + RewardsFacet(address(app)).transferERC721(baseContract, other, 1, activityId, activityType, ""); } } contract RewardFacet__integration_multicall is Setup { + address badgeContract; + + function _afterSetup() internal override { + // use app to create badge contract + vm.prank(creator); + badgeContract = ERC721FactoryFacet(address(app)).createERC721WithTokenURI( + name, symbol, baseURI, creator, uint16(tenPercentBPS), badgeImplementationId + ); + } + function test_can_multicall_minting_badges() public { bytes[] memory calls = new bytes[](2); calls[0] = @@ -305,7 +430,6 @@ contract RewardFacet__integration_multicall is Setup { } contract RewardsFacet__integration_mintERC20 is Setup { - event TokenMinted(address token, address to, uint256 amount, bytes32 id, bytes32 activityType, string uri); address tokenContract; function _afterSetup() internal override { @@ -362,7 +486,7 @@ contract RewardsFacet__integration_mintERC20 is Setup { ERC20Base(tokenContract).grantRole(MINTER_ROLE, other); vm.prank(other); - vm.expectRevert(); + vm.expectRevert(RewardsFacet.RewardsFacet_NotAuthorized.selector); RewardsFacet(address(app)).mintERC20( tokenContract, other, @@ -381,7 +505,7 @@ contract RewardsFacet__integration_mintERC20 is Setup { ERC20Base(tokenContract).revokeRole(ADMIN_ROLE, creator); vm.prank(other); - vm.expectRevert(); + vm.expectRevert(RewardsFacet.RewardsFacet_NotAuthorized.selector); RewardsFacet(address(app)).mintERC20( tokenContract, other, @@ -394,8 +518,6 @@ contract RewardsFacet__integration_mintERC20 is Setup { } contract RewardsFacet__integration_transferERC20 is Setup { - event TokenTransferred(address token, address to, uint256 amount, bytes32 id, bytes32 activityType, string uri); - address tokenContract; function _afterSetup() internal override { @@ -463,7 +585,7 @@ contract RewardsFacet__integration_transferERC20 is Setup { ERC20Base(tokenContract).approve(address(app), oneEther); vm.prank(other); - vm.expectRevert(); + vm.expectRevert(RewardsFacet.RewardsFacet_NotAuthorized.selector); RewardsFacet(address(app)).transferERC20( tokenContract, other,