From fe08730ec0573664ace110056d133ae95071a925 Mon Sep 17 00:00:00 2001 From: Krishang Nadgauda Date: Tue, 26 Apr 2022 14:49:53 -0400 Subject: [PATCH 1/3] add bot check --- contracts/drop/DropERC1155.sol | 4 +++- contracts/drop/DropERC20.sol | 4 +++- contracts/drop/DropERC721.sol | 4 +++- docs/DropERC1155.md | 17 ----------------- docs/DropERC721.md | 17 ----------------- 5 files changed, 9 insertions(+), 37 deletions(-) diff --git a/contracts/drop/DropERC1155.sol b/contracts/drop/DropERC1155.sol index 72860adb1..909932b20 100644 --- a/contracts/drop/DropERC1155.sol +++ b/contracts/drop/DropERC1155.sol @@ -59,7 +59,7 @@ contract DropERC1155 is uint256 private constant MAX_BPS = 10_000; /// @dev The thirdweb contract with fee related information. - ITWFee public immutable thirdwebFee; + ITWFee private immutable thirdwebFee; /// @dev Owner of the contract (purpose: OpenSea compatibility) address private _owner; @@ -254,6 +254,8 @@ contract DropERC1155 is bytes32[] calldata _proofs, uint256 _proofMaxQuantityPerTransaction ) external payable nonReentrant { + require(isTrustedForwarder(msg.sender) || _msgSender() == tx.origin, "BOT"); + // Get the active claim condition index. uint256 activeConditionId = getActiveClaimConditionId(_tokenId); diff --git a/contracts/drop/DropERC20.sol b/contracts/drop/DropERC20.sol index a496c7e72..7ab2605f3 100644 --- a/contracts/drop/DropERC20.sol +++ b/contracts/drop/DropERC20.sol @@ -47,7 +47,7 @@ contract DropERC20 is bytes32 private constant TRANSFER_ROLE = keccak256("TRANSFER_ROLE"); /// @dev The thirdweb contract with fee related information. - ITWFee internal immutable thirdwebFee; + ITWFee private immutable thirdwebFee; /// @dev Contract level metadata. string public contractURI; @@ -178,6 +178,8 @@ contract DropERC20 is bytes32[] calldata _proofs, uint256 _proofMaxQuantityPerTransaction ) external payable nonReentrant { + require(isTrustedForwarder(msg.sender) || _msgSender() == tx.origin, "BOT"); + // Get the claim conditions. uint256 activeConditionId = getActiveClaimConditionId(); diff --git a/contracts/drop/DropERC721.sol b/contracts/drop/DropERC721.sol index 2611550b7..44e556218 100644 --- a/contracts/drop/DropERC721.sol +++ b/contracts/drop/DropERC721.sol @@ -52,7 +52,7 @@ contract DropERC721 is uint256 private constant MAX_BPS = 10_000; /// @dev The thirdweb contract with fee related information. - ITWFee public immutable thirdwebFee; + ITWFee private immutable thirdwebFee; /// @dev Owner of the contract (purpose: OpenSea compatibility) address private _owner; @@ -317,6 +317,8 @@ contract DropERC721 is bytes32[] calldata _proofs, uint256 _proofMaxQuantityPerTransaction ) external payable nonReentrant { + require(isTrustedForwarder(msg.sender) || _msgSender() == tx.origin, "BOT"); + uint256 tokenIdToClaim = nextTokenIdToClaim; // Get the claim conditions. diff --git a/docs/DropERC1155.md b/docs/DropERC1155.md index 16978ff22..ff2b2d5fa 100644 --- a/docs/DropERC1155.md +++ b/docs/DropERC1155.md @@ -1008,23 +1008,6 @@ function symbol() external view returns (string) |---|---|---| | _0 | string | undefined -### thirdwebFee - -```solidity -function thirdwebFee() external view returns (contract ITWFee) -``` - - - -*The thirdweb contract with fee related information.* - - -#### Returns - -| Name | Type | Description | -|---|---|---| -| _0 | contract ITWFee | undefined - ### totalSupply ```solidity diff --git a/docs/DropERC721.md b/docs/DropERC721.md index 1393e0734..b3abe591e 100644 --- a/docs/DropERC721.md +++ b/docs/DropERC721.md @@ -1080,23 +1080,6 @@ function symbol() external view returns (string) |---|---|---| | _0 | string | undefined -### thirdwebFee - -```solidity -function thirdwebFee() external view returns (contract ITWFee) -``` - - - -*The thirdweb contract with fee related information.* - - -#### Returns - -| Name | Type | Description | -|---|---|---| -| _0 | contract ITWFee | undefined - ### tokenByIndex ```solidity From 5d9373012ef1b0e1faa6d8be2167a2d68b27d109 Mon Sep 17 00:00:00 2001 From: Krishang Nadgauda Date: Tue, 26 Apr 2022 15:17:21 -0400 Subject: [PATCH 2/3] test for fix --- lib/forge-std | 2 +- src/test/drop/DropERC721.t.sol | 100 ++++++++++++++++++++++++++++++++- 2 files changed, 98 insertions(+), 4 deletions(-) diff --git a/lib/forge-std b/lib/forge-std index 9401f0783..bdd321fc4 160000 --- a/lib/forge-std +++ b/lib/forge-std @@ -1 +1 @@ -Subproject commit 9401f07830e31937c9cfa77890c99c2ee50a5229 +Subproject commit bdd321fc4ac81aff81924effb559427fcccca769 diff --git a/src/test/drop/DropERC721.t.sol b/src/test/drop/DropERC721.t.sol index f2acb352a..f9b925850 100644 --- a/src/test/drop/DropERC721.t.sol +++ b/src/test/drop/DropERC721.t.sol @@ -6,7 +6,63 @@ import "contracts/drop/DropERC721.sol"; // Test imports import "../utils/BaseTest.sol"; -contract BaseDropERC721Test is BaseTest { +contract SubExploitContract is ERC721Holder, ERC1155Holder { + + DropERC721 internal drop; + address payable internal master; + + constructor(address _drop) { + drop = DropERC721(_drop); + master = payable(msg.sender); + } + + /// @dev Lets an account claim NFTs. + function claimDrop( + address _receiver, + uint256 _quantity, + address _currency, + uint256 _pricePerToken, + bytes32[] calldata _proofs, + uint256 _proofMaxQuantityPerTransaction + ) external { + drop.claim( + _receiver, + _quantity, + _currency, + _pricePerToken, + _proofs, + _proofMaxQuantityPerTransaction + ); + + selfdestruct(master); + } +} + +contract MasterExploitContract is ERC721Holder, ERC1155Holder { + + address internal drop; + + constructor(address _drop) { + drop = _drop; + } + + /// @dev Lets an account claim NFTs. + function performExploit( + address _receiver, + uint256 _quantity, + address _currency, + uint256 _pricePerToken, + bytes32[] calldata _proofs, + uint256 _proofMaxQuantityPerTransaction + ) external { + for(uint256 i = 0; i < 100; i ++) { + SubExploitContract sub = new SubExploitContract(address(drop)); + sub.claimDrop(_receiver, _quantity, _currency, _pricePerToken, _proofs, _proofMaxQuantityPerTransaction); + } + } +} + +contract DropERC721Test is BaseTest { DropERC721 public drop; function setUp() public override { @@ -99,7 +155,6 @@ contract BaseDropERC721Test is BaseTest { } function test_claimCondition_waitTimeInSecondsBetweenClaims() public { - vm.startPrank(deployer); vm.warp(1); address receiver = getActor(0); @@ -110,16 +165,20 @@ contract BaseDropERC721Test is BaseTest { conditions[0].quantityLimitPerTransaction = 100; conditions[0].waitTimeInSecondsBetweenClaims = type(uint256).max; + vm.prank(deployer); drop.lazyMint(100, "ipfs://", bytes("")); + vm.prank(deployer); drop.setClaimConditions(conditions, false); + + vm.prank(getActor(5), getActor(5)); drop.claim(receiver, 1, address(0), 0, proofs, 0); vm.expectRevert("cannot claim."); + vm.prank(getActor(5), getActor(5)); drop.claim(receiver, 1, address(0), 0, proofs, 0); } function test_claimCondition_resetEligibility_waitTimeInSecondsBetweenClaims() public { - vm.startPrank(deployer); vm.warp(1); address receiver = getActor(0); @@ -130,12 +189,47 @@ contract BaseDropERC721Test is BaseTest { conditions[0].quantityLimitPerTransaction = 100; conditions[0].waitTimeInSecondsBetweenClaims = type(uint256).max; + vm.prank(deployer); drop.lazyMint(100, "ipfs://", bytes("")); + vm.prank(deployer); drop.setClaimConditions(conditions, false); + + vm.prank(getActor(5), getActor(5)); drop.claim(receiver, 1, address(0), 0, proofs, 0); + vm.prank(deployer); drop.setClaimConditions(conditions, true); + + vm.prank(getActor(5), getActor(5)); drop.claim(receiver, 1, address(0), 0, proofs, 0); } + + function test_multiple_claim_exploit() public { + MasterExploitContract masterExploit = new MasterExploitContract(address(drop)); + + DropERC721.ClaimCondition[] memory conditions = new DropERC721.ClaimCondition[](1); + conditions[0].maxClaimableSupply = 100; + conditions[0].quantityLimitPerTransaction = 1; + conditions[0].waitTimeInSecondsBetweenClaims = type(uint256).max; + + vm.prank(deployer); + drop.lazyMint(100, "ipfs://", bytes("")); + + vm.prank(deployer); + drop.setClaimConditions(conditions, false); + + bytes32[] memory proofs = new bytes32[](0); + + vm.startPrank(getActor(5)); + vm.expectRevert(bytes("BOT")); + masterExploit.performExploit( + address(masterExploit), + conditions[0].quantityLimitPerTransaction, + conditions[0].currency, + conditions[0].pricePerToken, + proofs, + 0 + ); + } } From 5c4abdf1d242aa267b3b129698c6fe5085e7d894 Mon Sep 17 00:00:00 2001 From: Krishang Nadgauda Date: Tue, 26 Apr 2022 15:22:23 -0400 Subject: [PATCH 3/3] run prettier --- contracts/drop/DropERC1155.sol | 2 +- src/test/drop/DropERC721.t.sol | 15 +++------------ 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/contracts/drop/DropERC1155.sol b/contracts/drop/DropERC1155.sol index 909932b20..98e40b180 100644 --- a/contracts/drop/DropERC1155.sol +++ b/contracts/drop/DropERC1155.sol @@ -255,7 +255,7 @@ contract DropERC1155 is uint256 _proofMaxQuantityPerTransaction ) external payable nonReentrant { require(isTrustedForwarder(msg.sender) || _msgSender() == tx.origin, "BOT"); - + // Get the active claim condition index. uint256 activeConditionId = getActiveClaimConditionId(_tokenId); diff --git a/src/test/drop/DropERC721.t.sol b/src/test/drop/DropERC721.t.sol index f9b925850..36cf79abd 100644 --- a/src/test/drop/DropERC721.t.sol +++ b/src/test/drop/DropERC721.t.sol @@ -7,7 +7,6 @@ import "contracts/drop/DropERC721.sol"; import "../utils/BaseTest.sol"; contract SubExploitContract is ERC721Holder, ERC1155Holder { - DropERC721 internal drop; address payable internal master; @@ -25,21 +24,13 @@ contract SubExploitContract is ERC721Holder, ERC1155Holder { bytes32[] calldata _proofs, uint256 _proofMaxQuantityPerTransaction ) external { - drop.claim( - _receiver, - _quantity, - _currency, - _pricePerToken, - _proofs, - _proofMaxQuantityPerTransaction - ); + drop.claim(_receiver, _quantity, _currency, _pricePerToken, _proofs, _proofMaxQuantityPerTransaction); selfdestruct(master); } } contract MasterExploitContract is ERC721Holder, ERC1155Holder { - address internal drop; constructor(address _drop) { @@ -55,7 +46,7 @@ contract MasterExploitContract is ERC721Holder, ERC1155Holder { bytes32[] calldata _proofs, uint256 _proofMaxQuantityPerTransaction ) external { - for(uint256 i = 0; i < 100; i ++) { + for (uint256 i = 0; i < 100; i++) { SubExploitContract sub = new SubExploitContract(address(drop)); sub.claimDrop(_receiver, _quantity, _currency, _pricePerToken, _proofs, _proofMaxQuantityPerTransaction); } @@ -169,7 +160,7 @@ contract DropERC721Test is BaseTest { drop.lazyMint(100, "ipfs://", bytes("")); vm.prank(deployer); drop.setClaimConditions(conditions, false); - + vm.prank(getActor(5), getActor(5)); drop.claim(receiver, 1, address(0), 0, proofs, 0);