From 9b20060f08665c85186467d98bb8e697cab259ee Mon Sep 17 00:00:00 2001 From: jason li Date: Mon, 5 Feb 2024 14:07:17 +1100 Subject: [PATCH] finish tests --- contracts/mocks/MockFactory.sol | 20 ++ .../AllowlistERC721TransferApprovals.t.sol | 43 +++- .../AllowlistERC721TransfersApprovals.test.ts | 232 ------------------ test/utils/DeployFakeEOA.sol | 23 -- test/utils/DeployMockMarketPlace.sol | 2 +- test/utils/DeployRegularFixtures.ts | 1 - 6 files changed, 61 insertions(+), 260 deletions(-) delete mode 100644 test/allowlist/AllowlistERC721TransfersApprovals.test.ts delete mode 100644 test/utils/DeployFakeEOA.sol diff --git a/contracts/mocks/MockFactory.sol b/contracts/mocks/MockFactory.sol index 67858771..5ee8ee68 100644 --- a/contracts/mocks/MockFactory.sol +++ b/contracts/mocks/MockFactory.sol @@ -2,8 +2,12 @@ pragma solidity 0.8.19; import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; +import {IERC721} from "@openzeppelin/contracts/token/ERC721/IERC721.sol"; contract MockFactory { + bytes private constant mockDisguisedEOABytecode = + hex"608060405234801561001057600080fd5b5060405161021338038061021383398101604081905261002f91610054565b600080546001600160a01b0319166001600160a01b0392909216919091179055610084565b60006020828403121561006657600080fd5b81516001600160a01b038116811461007d57600080fd5b9392505050565b610180806100936000396000f3fe608060405234801561001057600080fd5b50600436106100365760003560e01c80639d76ea581461003b578063e58ef8a81461006a575b600080fd5b60005461004e906001600160a01b031681565b6040516001600160a01b03909116815260200160405180910390f35b61007d61007836600461010e565b61007f565b005b6000546040516323b872dd60e01b81526001600160a01b038581166004830152848116602483015260448201849052909116906323b872dd90606401600060405180830381600087803b1580156100d557600080fd5b505af11580156100e9573d6000803e3d6000fd5b50505050505050565b80356001600160a01b038116811461010957600080fd5b919050565b60008060006060848603121561012357600080fd5b61012c846100f2565b925061013a602085016100f2565b915060408401359050925092509256fea2646970667358221220cc26e879b9dbccdd8ff34bda1c5675a4b1a8497cba91bea35b6b744a41374a9a64736f6c63430008130033"; + function computeAddress(bytes32 salt, bytes32 codeHash) public view returns (address) { return Create2.computeAddress(salt, codeHash); } @@ -12,4 +16,20 @@ contract MockFactory { // slither-disable-next-line unused-return Create2.deploy(0, salt, code); } + + function deployMockEOAWithERC721Address(IERC721 tokenAddress, bytes32 salt) external returns (address) { + bytes memory encodedParams = abi.encode(address(tokenAddress)); + bytes memory constructorBytecode = abi.encodePacked(bytes(mockDisguisedEOABytecode), encodedParams); + address mockDisguisedEOAAddress = Create2.deploy(0, salt, constructorBytecode); + + return mockDisguisedEOAAddress; + } + + function computeMockDisguisedEOAAddress(IERC721 tokenAddress, bytes32 salt) external view returns (address) { + bytes memory encodedParams = abi.encode(address(tokenAddress)); + bytes memory constructorBytecode = abi.encodePacked(bytes(mockDisguisedEOABytecode), encodedParams); + address computedAddress = Create2.computeAddress(salt, keccak256(constructorBytecode)); + + return computedAddress; + } } diff --git a/test/allowlist/AllowlistERC721TransferApprovals.t.sol b/test/allowlist/AllowlistERC721TransferApprovals.t.sol index 054a8ea3..a06d50d8 100644 --- a/test/allowlist/AllowlistERC721TransferApprovals.t.sol +++ b/test/allowlist/AllowlistERC721TransferApprovals.t.sol @@ -4,6 +4,7 @@ import "forge-std/Test.sol"; import {MockWallet} from "../../contracts/mocks/MockWallet.sol"; import {MockWalletFactory} from "../../contracts/mocks/MockWalletFactory.sol"; +import {MockFactory} from "../../contracts/mocks/MockFactory.sol"; import {ImmutableERC721} from "../../contracts/token/erc721/preset/ImmutableERC721.sol"; import {IImmutableERC721Errors} from "../../contracts/errors/Errors.sol"; import {OperatorAllowlistEnforcementErrors} from "../../contracts/errors/Errors.sol"; @@ -12,16 +13,18 @@ import {Sign} from "../utils/Sign.sol"; import {DeployOperatorAllowlist} from "../utils/DeployAllowlistProxy.sol"; import {DeploySCWallet} from "../utils/DeploySCW.sol"; import {DeployMockMarketPlace} from "../utils/DeployMockMarketPlace.sol"; -import {MockMarketplace} from "../../contracts/mocks/MockMarketPlace.sol"; -import {DeployFakeEOA} from "../utils/DeployFakeEOA.sol"; +import {MockMarketplace} from "../../contracts/mocks/MockMarketplace.sol"; +import {MockDisguisedEOA} from "../../contracts/mocks/MockDisguisedEOA.sol"; +import {MockOnReceive} from "../../contracts/mocks/MockOnReceive.sol"; + contract AllowlistERC721TransferApprovals is Test { OperatorAllowlistUpgradeable public allowlist; ImmutableERC721 public immutableERC721; DeploySCWallet public deploySCWScript; DeployMockMarketPlace public deployMockMarketPlaceScript; - DeployFakeEOA public deployFakeEOAScript; MockMarketplace public mockMarketPlace; + MockFactory mockEOAFactory; uint256 feeReceiverKey = 1; @@ -54,12 +57,15 @@ contract AllowlistERC721TransferApprovals is Test { 0 ); + mockEOAFactory = new MockFactory(); + nonAuthorizedWallet = address(0x2); deploySCWScript = new DeploySCWallet(); deployMockMarketPlaceScript = new DeployMockMarketPlace(); mockMarketPlace = deployMockMarketPlaceScript.run(address(immutableERC721)); + _giveMinterRole(); } @@ -226,6 +232,37 @@ contract AllowlistERC721TransferApprovals is Test { vm.stopPrank(); } + function testDisguisedEOAApprovalTransfer() public { + vm.startPrank(minter, minter); + immutableERC721.safeMint(minter, 1); + bytes32 salt = keccak256(abi.encodePacked("0x1234")); + + address create2Addr = mockEOAFactory.computeMockDisguisedEOAAddress(immutableERC721, salt); + + immutableERC721.setApprovalForAll(create2Addr, true); + mockEOAFactory.deployMockEOAWithERC721Address(immutableERC721, salt); + + assertTrue(immutableERC721.isApprovedForAll(minter, create2Addr)); + + MockDisguisedEOA disguisedEOA = MockDisguisedEOA(create2Addr); + + vm.expectRevert(abi.encodeWithSignature("CallerNotInAllowlist(address)", create2Addr)); + disguisedEOA.executeTransfer(minter, admin, 1); + vm.stopPrank(); + } + + // Here the malicious contract attempts to transfer the token out of the contract by calling transferFrom in onERC721Received + // However, sending to the contract will fail as the contract is not in the allowlist. + function testOnReceiveTransferFrom() public { + MockOnReceive onReceive = new MockOnReceive(immutableERC721, admin); + + vm.startPrank(minter, minter); + immutableERC721.safeMint(minter, 1); + vm.expectRevert(abi.encodeWithSignature("TransferToNotInAllowlist(address)", address(onReceive))); + immutableERC721.safeTransferFrom(minter, address(onReceive), 1, ""); + vm.stopPrank(); + } + } \ No newline at end of file diff --git a/test/allowlist/AllowlistERC721TransfersApprovals.test.ts b/test/allowlist/AllowlistERC721TransfersApprovals.test.ts deleted file mode 100644 index 74f84603..00000000 --- a/test/allowlist/AllowlistERC721TransfersApprovals.test.ts +++ /dev/null @@ -1,232 +0,0 @@ -import { expect } from "chai"; -import { ethers } from "hardhat"; -import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; -import { - ImmutableERC721MintByID, - MockMarketplace, - MockFactory, - OperatorAllowlist, - MockOnReceive, - MockOnReceive__factory, - MockWalletFactory, -} from "../../typechain-types"; -import { RegularAllowlistFixture, walletSCFixture, disguidedEOAFixture } from "../utils/DeployRegularFixtures"; - -describe("Allowlisted ERC721 Transfers", function () { - this.timeout(300_000); // 5 min - - let erc721: ImmutableERC721MintByID; - let walletFactory: MockWalletFactory; - let factory: MockFactory; - let operatorAllowlist: OperatorAllowlist; - let marketPlace: MockMarketplace; - let deployedAddr: string; // deployed SC wallet address - let moduleAddress: string; - let owner: SignerWithAddress; - let minter: SignerWithAddress; - let registrar: SignerWithAddress; - let scWallet: SignerWithAddress; - let accs: SignerWithAddress[]; - - beforeEach(async function () { - [owner, minter, registrar, scWallet, ...accs] = await ethers.getSigners(); - - // Get all required contracts - ({ erc721, walletFactory, factory, operatorAllowlist, marketPlace } = await RegularAllowlistFixture(owner)); - // Deploy the wallet fixture - ({ deployedAddr, moduleAddress } = await walletSCFixture(scWallet, walletFactory)); - - // Set up roles - await erc721.connect(owner).grantMinterRole(minter.address); - await operatorAllowlist.connect(owner).grantRegistrarRole(registrar.address); - }); - - describe("Operator Allowlist Registry setting", function () { - it("Should have operatorAllowlist set upon deployment", async function () { - expect(await erc721.operatorAllowlist()).to.equal(operatorAllowlist.address); - }); - }); - - describe("Approvals", function () { - it("Should not allow a non-Allowlisted operator to be approved", async function () { - await erc721.connect(minter).mint(minter.address, 1); - // Approve for all - await expect(erc721.connect(minter).setApprovalForAll(marketPlace.address, true)) - .to.be.revertedWith("ApproveTargetNotInAllowlist") - .withArgs(marketPlace.address); - // Approve - await expect(erc721.connect(minter).approve(marketPlace.address, 1)) - .to.be.revertedWith("ApproveTargetNotInAllowlist") - .withArgs(marketPlace.address); - }); - - it("Not allowlisted contracts should not be able to approve", async function () { - await erc721.connect(minter).mint(marketPlace.address, 2); - await expect(marketPlace.connect(minter).executeApproveForAll(minter.address, true)) - .to.be.revertedWith("ApproverNotInAllowlist") - .withArgs(marketPlace.address); - }); - - it("Should allow EOAs to be approved", async function () { - await erc721.connect(minter).mint(minter.address, 3); - await erc721.connect(minter).mint(minter.address, 1); - // Approve EOA addr - await erc721.connect(minter).approve(accs[0].address, 3); - await erc721.connect(minter).setApprovalForAll(accs[0].address, true); - expect(await erc721.getApproved(3)).to.be.equal(accs[0].address); - expect(await erc721.isApprovedForAll(minter.address, accs[0].address)).to.be.equal(true); - }); - - it("Should allow Allowlisted addresses to be approved", async function () { - // Add the mock marketplace to registry - await operatorAllowlist.connect(registrar).addAddressToAllowlist([marketPlace.address]); - // Approve marketplace on erc721 contract - await erc721.connect(minter).mint(minter.address, 2); - await erc721.connect(minter).approve(marketPlace.address, 2); - await erc721.connect(minter).setApprovalForAll(marketPlace.address, true); - expect(await erc721.getApproved(2)).to.be.equal(marketPlace.address); - expect(await erc721.isApprovedForAll(minter.address, marketPlace.address)).to.be.equal(true); - }); - - it("Should allow Allowlisted smart contract wallets to be approved", async function () { - // Allowlist the bytecode - await operatorAllowlist.connect(registrar).addWalletToAllowlist(deployedAddr); - await erc721.connect(minter).mint(minter.address, 3); - await erc721.connect(minter).approve(deployedAddr, 3); - // Approve the smart contract wallet - await erc721.connect(minter).setApprovalForAll(deployedAddr, true); - expect(await erc721.getApproved(3)).to.be.equal(deployedAddr); - expect(await erc721.isApprovedForAll(minter.address, deployedAddr)).to.be.equal(true); - }); - }); - - describe("Transfers", function () { - it("Should freely allow transfers between EOAs", async function () { - await erc721.connect(owner).grantMinterRole(accs[0].address); - await erc721.connect(owner).grantMinterRole(accs[1].address); - await erc721.connect(accs[0]).mint(accs[0].address, 1); - await erc721.connect(accs[1]).mint(accs[1].address, 2); - // Transfer - await erc721.connect(accs[0]).transferFrom(accs[0].address, accs[2].address, 1); - await erc721.connect(accs[1]).transferFrom(accs[1].address, accs[2].address, 2); - // Check balance - expect(await erc721.balanceOf(accs[2].address)).to.be.equal(2); - // Transfer again - await erc721.connect(accs[2]).transferFrom(accs[2].address, accs[0].address, 1); - await erc721.connect(accs[2]).transferFrom(accs[2].address, accs[1].address, 2); - // Check final balance - expect(await erc721.balanceOf(accs[2].address)).to.be.equal(0); - - // Approved EOA account should be able to transfer - await erc721.connect(accs[0]).setApprovalForAll(accs[2].address, true); - await erc721.connect(accs[2]).transferFrom(accs[0].address, accs[2].address, 1); - expect(await erc721.balanceOf(accs[2].address)).to.be.equal(1); - }); - - it("Should block transfers from a not allow listed contracts", async function () { - await erc721.connect(minter).mint(marketPlace.address, 5); - await expect(marketPlace.connect(minter).executeTransferFrom(marketPlace.address, minter.address, 5)) - .to.be.revertedWith("CallerNotInAllowlist") - .withArgs(marketPlace.address); - }); - - it("Should block transfers to a not allow listed address", async function () { - await erc721.connect(minter).mint(minter.address, 1); - await expect(erc721.connect(minter).transferFrom(minter.address, marketPlace.address, 1)) - .to.be.revertedWith("TransferToNotInAllowlist") - .withArgs(marketPlace.address); - }); - - it("Should not block transfers from an allow listed contract", async function () { - await operatorAllowlist.connect(registrar).addAddressToAllowlist([marketPlace.address]); - await erc721.connect(minter).mint(minter.address, 4); - await erc721.connect(minter).setApprovalForAll(marketPlace.address, true); - expect(await erc721.balanceOf(accs[3].address)).to.be.equal(0); - await marketPlace.connect(minter).executeTransfer(accs[3].address, 4); - expect(await erc721.balanceOf(accs[3].address)).to.be.equal(1); - }); - - it("Should not block transfers between allow listed smart contract wallets", async function () { - // Deploy more SC wallets - const salt = ethers.utils.keccak256("0x4567"); - const saltTwo = ethers.utils.keccak256("0x5678"); - const saltThree = ethers.utils.keccak256("0x6789"); - await walletFactory.connect(scWallet).deploy(moduleAddress, salt); - await walletFactory.connect(scWallet).deploy(moduleAddress, saltTwo); - await walletFactory.connect(scWallet).deploy(moduleAddress, saltThree); - const deployedAddr = await walletFactory.getAddress(moduleAddress, salt); - - await operatorAllowlist.connect(registrar).addWalletToAllowlist(deployedAddr); - - const deployedAddrTwo = await walletFactory.getAddress(moduleAddress, saltTwo); - const deployedAddrThree = await walletFactory.getAddress(moduleAddress, saltThree); - // Mint NFTs to the wallets - await erc721.connect(minter).mint(deployedAddr, 10); - await erc721.connect(minter).mint(deployedAddrTwo, 11); - - // Connect to wallets - const wallet = await ethers.getContractAt("MockWallet", deployedAddr); - const walletTwo = await ethers.getContractAt("MockWallet", deployedAddrTwo); - const walletThree = await ethers.getContractAt("MockWallet", deployedAddrThree); - - // Transfer between wallets - await wallet.transferNFT(erc721.address, deployedAddr, deployedAddrThree, 10); - await walletTwo.transferNFT(erc721.address, deployedAddrTwo, deployedAddrThree, 11); - expect(await erc721.balanceOf(deployedAddr)).to.be.equal(0); - expect(await erc721.balanceOf(deployedAddrTwo)).to.be.equal(0); - expect(await erc721.balanceOf(deployedAddrThree)).to.be.equal(2); - await walletThree.transferNFT(erc721.address, deployedAddrThree, deployedAddr, 10); - await walletThree.transferNFT(erc721.address, deployedAddrThree, deployedAddrTwo, 11); - expect(await erc721.balanceOf(deployedAddr)).to.be.equal(1); - expect(await erc721.balanceOf(deployedAddrTwo)).to.be.equal(1); - expect(await erc721.balanceOf(deployedAddrThree)).to.be.equal(0); - }); - }); - - describe("Malicious Contracts", function () { - // The EOA disguise attack vector is a where a pre-computed CREATE2 deterministic address is disguised as an EOA. - // By virtue of this, approvals and transfers to this address will pass. We need to catch actions from this address - // once it is deployed. - it("EOA disguise approve", async function () { - // This attack vector is where a CFA is approved prior to deployment. This passes as at the time of approval as - // the CFA is treated as an EOA, passing _validateApproval. - // This means post-deployment that the address is now an approved operator - // and is able to call transferFrom. - const { deployedAddr, salt, constructorByteCode } = await disguidedEOAFixture(erc721.address, factory, "0x1234"); - // Approve disguised EOA - await erc721.connect(minter).mint(minter.address, 1); - await erc721.connect(minter).setApprovalForAll(deployedAddr, true); - // Deploy disguised EOA - await factory.connect(accs[5]).deploy(salt, constructorByteCode); - expect(await erc721.isApprovedForAll(minter.address, deployedAddr)).to.be.equal(true); - // Attempt to execute a transferFrom, w/ msg.sender being the disguised EOA - const disguisedEOAFactory = ethers.getContractFactory("MockDisguisedEOA"); - const disguisedEOA = (await disguisedEOAFactory).attach(deployedAddr); - // Catch transfer as msg.sender != tx.origin - await expect(disguisedEOA.connect(minter).executeTransfer(minter.address, accs[5].address, 1)) - .to.be.revertedWith("CallerNotInAllowlist") - .withArgs(deployedAddr); - }); - - it("EOA disguise transferFrom", async function () { - // This vector is where the NFT is transferred to the CFA and executes a transferFrom inside its constructor. - // TODO: investigate why transferFrom calls fail within the constructor. This will be caught as msg.sender != tx.origin. - }); - - // Here the malicious contract attempts to transfer the token out of the contract by calling transferFrom in onERC721Received - // However, sending to the contract will fail as the contract is not in the allowlist. - it("onRecieve transferFrom", async function () { - // Deploy contract - const mockOnReceiveFactory = (await ethers.getContractFactory("MockOnReceive")) as MockOnReceive__factory; - const onRecieve: MockOnReceive = await mockOnReceiveFactory.deploy(erc721.address, accs[6].address); - // Mint and transfer to receiver contract - await erc721.connect(minter).mint(minter.address, 1); - // Fails as transfer 'to' is now allowlisted - await expect( - erc721.connect(minter)["safeTransferFrom(address,address,uint256)"](minter.address, onRecieve.address, 1) - ) - .to.be.revertedWith("TransferToNotInAllowlist") - .withArgs(onRecieve.address); - }); - }); -}); diff --git a/test/utils/DeployFakeEOA.sol b/test/utils/DeployFakeEOA.sol deleted file mode 100644 index d6fe1902..00000000 --- a/test/utils/DeployFakeEOA.sol +++ /dev/null @@ -1,23 +0,0 @@ -// SPDX-License-Identifier: Apache 2.0 -pragma solidity ^0.8.19; - -import "forge-std/Test.sol"; -import {MockDisguisedEOA} from "../../contracts/mocks/MockDisguisedEOA.sol"; -import {MockFactory} from "../../contracts/mocks/MockFactory.sol"; - -contract DeployFakeEOA is Test { - function run(address erc721, string memory saltString) external returns (address, bytes32, bytes32) { - MockFactory factory = new MockFactory(); - bytes memory encodedParams = abi.encodePacked(erc721); - - bytes32 salt = keccak256(abi.encodePacked(saltString)); - - bytes memory mockDisguisedEOABytecode = "0x608060405234801561001057600080fd5b5060405161021338038061021383398101604081905261002f91610054565b600080546001600160a01b0319166001600160a01b0392909216919091179055610084565b60006020828403121561006657600080fd5b81516001600160a01b038116811461007d57600080fd5b9392505050565b610180806100936000396000f3fe608060405234801561001057600080fd5b50600436106100365760003560e01c80639d76ea581461003b578063e58ef8a81461006a575b600080fd5b60005461004e906001600160a01b031681565b6040516001600160a01b03909116815260200160405180910390f35b61007d61007836600461010e565b61007f565b005b6000546040516323b872dd60e01b81526001600160a01b038581166004830152848116602483015260448201849052909116906323b872dd90606401600060405180830381600087803b1580156100d557600080fd5b505af11580156100e9573d6000803e3d6000fd5b50505050505050565b80356001600160a01b038116811461010957600080fd5b919050565b60008060006060848603121561012357600080fd5b61012c846100f2565b925061013a602085016100f2565b915060408401359050925092509256fea2646970667358221220cc26e879b9dbccdd8ff34bda1c5675a4b1a8497cba91bea35b6b744a41374a9a64736f6c63430008130033"; - - // Append bytecode and constructor params - bytes32 constructorByteCode = keccak256(abi.encodePacked(mockDisguisedEOABytecode, encodedParams)); - - address create2Addr = factory.computeAddress(salt, constructorByteCode); - return (create2Addr, salt, constructorByteCode); - } -} \ No newline at end of file diff --git a/test/utils/DeployMockMarketPlace.sol b/test/utils/DeployMockMarketPlace.sol index 09f213db..ec8764b7 100644 --- a/test/utils/DeployMockMarketPlace.sol +++ b/test/utils/DeployMockMarketPlace.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: Apache 2.0 pragma solidity ^0.8.19; -import {MockMarketplace} from "../../contracts/mocks/MockMarketPlace.sol"; +import {MockMarketplace} from "../../contracts/mocks/MockMarketplace.sol"; /// Deploys the OperatorAllowlistUpgradeable contract behind an ERC1967 Proxy and returns the address of the proxy diff --git a/test/utils/DeployRegularFixtures.ts b/test/utils/DeployRegularFixtures.ts index 538190bd..2ebb2ed3 100644 --- a/test/utils/DeployRegularFixtures.ts +++ b/test/utils/DeployRegularFixtures.ts @@ -98,7 +98,6 @@ export const disguidedEOAFixture = async (erc721Addr: string, MockFactory: MockF // Append bytecode and constructor params const constructorByteCode = `${mockDisguisedEOAArtifact.bytecode}${encodedParams}`; - console.log(mockDisguisedEOAArtifact.bytecode) // Calulate address of deployed contract const deployedAddr = await MockFactory.computeAddress(salt, ethers.utils.keccak256(constructorByteCode));