diff --git a/src/Factory.sol b/src/Factory.sol index 23e75cbd..6c52470e 100644 --- a/src/Factory.sol +++ b/src/Factory.sol @@ -171,8 +171,11 @@ contract Factory is IFactory, ERC721, FactoryGuardian { * @param account The address of the Account that is transferred. * @dev This method transfers an Account on Account address instead of id and * also transfers the Account proxy contract to the new owner. + * @dev The Account itself cannot become its owner. */ function safeTransferFrom(address from, address to, address account) public { + if (to == account) revert FactoryErrors.InvalidRecipient(); + uint256 id = accountIndex[account]; IAccount(allAccounts[id - 1]).transferOwnership(to); super.safeTransferFrom(from, to, id); @@ -185,9 +188,13 @@ contract Factory is IFactory, ERC721, FactoryGuardian { * @param id The id of the Account that is about to be transferred. * @dev This method overwrites the safeTransferFrom function in ERC721.sol to * also transfer the Account proxy contract to the new owner. + * @dev The Account itself cannot become its owner. */ function safeTransferFrom(address from, address to, uint256 id) public override { - IAccount(allAccounts[id - 1]).transferOwnership(to); + address account = allAccounts[id - 1]; + if (to == account) revert FactoryErrors.InvalidRecipient(); + + IAccount(account).transferOwnership(to); super.safeTransferFrom(from, to, id); } @@ -199,9 +206,13 @@ contract Factory is IFactory, ERC721, FactoryGuardian { * @param data additional data, only used for onERC721Received. * @dev This method overwrites the safeTransferFrom function in ERC721.sol to * also transfer the Account proxy contract to the new owner. + * @dev The Account itself cannot become its owner. */ function safeTransferFrom(address from, address to, uint256 id, bytes calldata data) public override { - IAccount(allAccounts[id - 1]).transferOwnership(to); + address account = allAccounts[id - 1]; + if (to == account) revert FactoryErrors.InvalidRecipient(); + + IAccount(account).transferOwnership(to); super.safeTransferFrom(from, to, id, data); } @@ -212,9 +223,13 @@ contract Factory is IFactory, ERC721, FactoryGuardian { * @param id The id of the Account that is about to be transferred. * @dev This method overwrites the transferFrom function in ERC721.sol to * also transfer the Account proxy contract to the new owner. + * @dev The Account itself cannot become its owner. */ function transferFrom(address from, address to, uint256 id) public override { - IAccount(allAccounts[id - 1]).transferOwnership(to); + address account = allAccounts[id - 1]; + if (to == account) revert FactoryErrors.InvalidRecipient(); + + IAccount(account).transferOwnership(to); super.transferFrom(from, to, id); } @@ -223,9 +238,11 @@ contract Factory is IFactory, ERC721, FactoryGuardian { * @param to The target. * @dev Adaptation of safeTransferFrom from the ERC-721 standard, where the Account itself triggers the transfer. * @dev The Account must do the transferOwnership() before calling this function. + * @dev The Account itself cannot become its owner. */ function safeTransferAccount(address to) public { if (to == address(0)) revert FactoryErrors.InvalidRecipient(); + if (to == msg.sender) revert FactoryErrors.InvalidRecipient(); uint256 id = accountIndex[msg.sender]; if (id == 0) revert FactoryErrors.OnlyAccount(); diff --git a/test/fuzz/Factory/SafeTransferAccount.fuzz.sol b/test/fuzz/Factory/SafeTransferAccount.fuzz.sol index 8288287f..0b173b8d 100644 --- a/test/fuzz/Factory/SafeTransferAccount.fuzz.sol +++ b/test/fuzz/Factory/SafeTransferAccount.fuzz.sol @@ -23,14 +23,21 @@ contract SafeTransferAccount_Factory_Fuzz_Test is Factory_Fuzz_Test { /*////////////////////////////////////////////////////////////// TESTS //////////////////////////////////////////////////////////////*/ - function testFuzz_Revert_safeTransferAccount_InvalidRecipient(address sender) public { + function testFuzz_Revert_safeTransferAccount_ToZeroAddress(address sender) public { vm.prank(sender); vm.expectRevert(FactoryErrors.InvalidRecipient.selector); factory.safeTransferAccount(address(0)); } + function testFuzz_Revert_safeTransferAccount_ToAccount() public { + vm.prank(address(proxyAccount)); + vm.expectRevert(FactoryErrors.InvalidRecipient.selector); + factory.safeTransferAccount(address(proxyAccount)); + } + function testFuzz_Revert_safeTransferAccount_NonAccount(address sender, address to) public { vm.assume(to != address(0)); + vm.assume(to != address(proxyAccount)); vm.assume(sender != address(proxyAccount)); vm.prank(sender); @@ -52,6 +59,7 @@ contract SafeTransferAccount_Factory_Fuzz_Test is Factory_Fuzz_Test { function testFuzz_Success_safeTransferAccount(address to) public canReceiveERC721(to) { vm.assume(to != users.accountOwner); vm.assume(to != address(0)); + vm.assume(to != address(proxyAccount)); uint256 balanceOwnerBefore = factory.balanceOf(users.accountOwner); uint256 balanceToBefore = factory.balanceOf(to); diff --git a/test/fuzz/Factory/TransferFrom.fuzz.sol b/test/fuzz/Factory/TransferFrom.fuzz.sol index bb420e34..98860bab 100644 --- a/test/fuzz/Factory/TransferFrom.fuzz.sol +++ b/test/fuzz/Factory/TransferFrom.fuzz.sol @@ -4,7 +4,7 @@ */ pragma solidity 0.8.22; -import { Factory_Fuzz_Test } from "./_Factory.fuzz.t.sol"; +import { Factory_Fuzz_Test, FactoryErrors } from "./_Factory.fuzz.t.sol"; import { AccountV1 } from "../../../src/accounts/AccountV1.sol"; import { AccountExtension } from "../../utils/Extensions.sol"; @@ -34,12 +34,6 @@ contract TransferFrom_Factory_Fuzz_Test is Factory_Fuzz_Test { HELPER FUNCTIONS //////////////////////////////////////////////////////////////*/ - modifier notAccountOwner(address accountOwner) { - vm.assume(accountOwner != address(0)); - vm.assume(accountOwner != users.accountOwner); - _; - } - function coolDownPeriodPassed(address account, uint32 lastActionTimestamp, uint32 timePassed) public { AccountV1 account_ = AccountV1(account); timePassed = uint32(bound(timePassed, coolDownPeriod + 1, type(uint32).max)); @@ -56,207 +50,331 @@ contract TransferFrom_Factory_Fuzz_Test is Factory_Fuzz_Test { /*////////////////////////////////////////////////////////////// TESTS //////////////////////////////////////////////////////////////*/ - function testFuzz_Revert_SFT1_InvalidRecipient( - address newAccountOwner, + function testFuzz_Revert_SFT1_ToZeroAddress( + address owner, uint32 salt, uint32 lastActionTimestamp, uint32 timePassed - ) public notAccountOwner(newAccountOwner) { - vm.broadcast(newAccountOwner); + ) public { + vm.assume(owner != address(0)); + + vm.broadcast(owner); address newAccount = factory.createAccount(salt, 0, address(0)); coolDownPeriodPassed(newAccount, lastActionTimestamp, timePassed); - vm.prank(newAccountOwner); + vm.prank(owner); vm.expectRevert("INVALID_RECIPIENT"); - factory.safeTransferFrom(newAccountOwner, address(0), newAccount); + factory.safeTransferFrom(owner, address(0), newAccount); + } + + function testFuzz_Revert_SFT1_ToAccount(address owner, uint32 salt, uint32 lastActionTimestamp, uint32 timePassed) + public + { + vm.assume(owner != address(0)); + + vm.broadcast(owner); + address newAccount = factory.createAccount(salt, 0, address(0)); + + coolDownPeriodPassed(newAccount, lastActionTimestamp, timePassed); + + vm.prank(owner); + vm.expectRevert(FactoryErrors.InvalidRecipient.selector); + factory.safeTransferFrom(owner, newAccount, newAccount); } function testFuzz_Revert_SFT1_CallerNotOwner( - address newAccountOwner, - address nonOwner, + address owner, + address caller, + address to, uint32 salt, uint32 lastActionTimestamp, uint32 timePassed - ) public notAccountOwner(newAccountOwner) { - vm.broadcast(newAccountOwner); + ) public { + vm.assume(owner != address(0)); + vm.assume(owner != caller); + + vm.broadcast(owner); address newAccount = factory.createAccount(salt, 0, address(0)); + vm.assume(to != newAccount); + coolDownPeriodPassed(newAccount, lastActionTimestamp, timePassed); - vm.prank(nonOwner); - vm.expectRevert("WRONG_FROM"); - factory.safeTransferFrom(users.accountOwner, nonOwner, newAccount); + vm.prank(caller); + vm.expectRevert("NOT_AUTHORIZED"); + factory.safeTransferFrom(owner, to, newAccount); } - function testFuzz_Revert_SFT2_InvalidRecipient( - address newAccountOwner, + function testFuzz_Revert_SFT2_ToZeroAddress( + address owner, uint32 salt, uint32 lastActionTimestamp, uint32 timePassed - ) public notAccountOwner(newAccountOwner) { - vm.broadcast(newAccountOwner); + ) public { + vm.assume(owner != address(0)); + + vm.broadcast(owner); address newAccount = factory.createAccount(salt, 0, address(0)); coolDownPeriodPassed(newAccount, lastActionTimestamp, timePassed); uint256 latestId = factory.allAccountsLength(); - vm.prank(newAccountOwner); + vm.prank(owner); vm.expectRevert("INVALID_RECIPIENT"); - factory.safeTransferFrom(newAccountOwner, address(0), latestId); + factory.safeTransferFrom(owner, address(0), latestId); + } + + function testFuzz_Revert_SFT2_InvalidRecipient( + address owner, + uint32 salt, + uint32 lastActionTimestamp, + uint32 timePassed + ) public { + vm.assume(owner != address(0)); + + vm.broadcast(owner); + address newAccount = factory.createAccount(salt, 0, address(0)); + + coolDownPeriodPassed(newAccount, lastActionTimestamp, timePassed); + + uint256 latestId = factory.allAccountsLength(); + vm.prank(owner); + vm.expectRevert(FactoryErrors.InvalidRecipient.selector); + factory.safeTransferFrom(owner, newAccount, latestId); } function testFuzz_Revert_SFT2_CallerNotOwner( - address newAccountOwner, - address nonOwner, + address owner, + address caller, + address to, uint32 salt, uint32 lastActionTimestamp, uint32 timePassed - ) public notAccountOwner(newAccountOwner) { - vm.broadcast(newAccountOwner); + ) public { + vm.assume(owner != address(0)); + vm.assume(owner != caller); + + vm.broadcast(owner); address newAccount = factory.createAccount(salt, 0, address(0)); + vm.assume(to != newAccount); + coolDownPeriodPassed(newAccount, lastActionTimestamp, timePassed); uint256 latestId = factory.allAccountsLength(); - vm.prank(nonOwner); - vm.expectRevert("WRONG_FROM"); - factory.safeTransferFrom(users.accountOwner, nonOwner, latestId); + vm.prank(caller); + vm.expectRevert("NOT_AUTHORIZED"); + factory.safeTransferFrom(owner, to, latestId); } - function testFuzz_Revert_SFT3_InvalidRecipient( - address newAccountOwner, + function testFuzz_Revert_SFT3_ToZeroAddress( + address owner, uint32 salt, uint32 lastActionTimestamp, uint32 timePassed - ) public notAccountOwner(newAccountOwner) { - vm.broadcast(newAccountOwner); + ) public { + vm.assume(owner != address(0)); + + vm.broadcast(owner); address newAccount = factory.createAccount(salt, 0, address(0)); coolDownPeriodPassed(newAccount, lastActionTimestamp, timePassed); uint256 latestId = factory.allAccountsLength(); - vm.prank(newAccountOwner); + vm.prank(owner); vm.expectRevert("INVALID_RECIPIENT"); - factory.safeTransferFrom(newAccountOwner, address(0), latestId, ""); + factory.safeTransferFrom(owner, address(0), latestId, ""); + } + + function testFuzz_Revert_SFT3_ToAccount(address owner, uint32 salt, uint32 lastActionTimestamp, uint32 timePassed) + public + { + vm.assume(owner != address(0)); + + vm.broadcast(owner); + address newAccount = factory.createAccount(salt, 0, address(0)); + + coolDownPeriodPassed(newAccount, lastActionTimestamp, timePassed); + + uint256 latestId = factory.allAccountsLength(); + vm.prank(owner); + vm.expectRevert(FactoryErrors.InvalidRecipient.selector); + factory.safeTransferFrom(owner, newAccount, latestId, ""); } function testFuzz_Revert_SFT3_CallerNotOwner( - address newAccountOwner, - address nonOwner, + address owner, + address caller, + address to, uint32 salt, uint32 lastActionTimestamp, uint32 timePassed - ) public notAccountOwner(newAccountOwner) { - vm.broadcast(newAccountOwner); + ) public { + vm.assume(owner != address(0)); + vm.assume(owner != caller); + + vm.broadcast(owner); address newAccount = factory.createAccount(salt, 0, address(0)); + vm.assume(to != newAccount); + coolDownPeriodPassed(newAccount, lastActionTimestamp, timePassed); uint256 latestId = factory.allAccountsLength(); - vm.prank(nonOwner); - vm.expectRevert("WRONG_FROM"); - factory.safeTransferFrom(users.accountOwner, nonOwner, latestId, ""); + vm.prank(caller); + vm.expectRevert("NOT_AUTHORIZED"); + factory.safeTransferFrom(owner, to, latestId, ""); } - function testFuzz_Revert_TransferFrom_InvalidRecipient( - address newAccountOwner, + function testFuzz_Revert_TransferFrom_ToZeroAddress( + address owner, uint32 salt, uint32 lastActionTimestamp, uint32 timePassed - ) public notAccountOwner(newAccountOwner) { - vm.broadcast(newAccountOwner); + ) public { + vm.assume(owner != address(0)); + + vm.broadcast(owner); address newAccount = factory.createAccount(salt, 0, address(0)); coolDownPeriodPassed(newAccount, lastActionTimestamp, timePassed); uint256 latestId = factory.allAccountsLength(); - vm.prank(newAccountOwner); + vm.prank(owner); vm.expectRevert("INVALID_RECIPIENT"); - factory.transferFrom(newAccountOwner, address(0), latestId); + factory.transferFrom(owner, address(0), latestId); + } + + function testFuzz_Revert_TransferFrom_ToAccount( + address owner, + uint32 salt, + uint32 lastActionTimestamp, + uint32 timePassed + ) public { + vm.assume(owner != address(0)); + + vm.broadcast(owner); + address newAccount = factory.createAccount(salt, 0, address(0)); + + coolDownPeriodPassed(newAccount, lastActionTimestamp, timePassed); + + uint256 latestId = factory.allAccountsLength(); + vm.prank(owner); + vm.expectRevert(FactoryErrors.InvalidRecipient.selector); + factory.transferFrom(owner, newAccount, latestId); } function testFuzz_Revert_TransferFrom_CallerNotOwner( - address newAccountOwner, - address nonOwner, + address owner, + address caller, + address to, uint32 salt, uint32 lastActionTimestamp, uint32 timePassed - ) public notAccountOwner(newAccountOwner) { - vm.broadcast(newAccountOwner); + ) public { + vm.assume(owner != address(0)); + vm.assume(owner != caller); + + vm.broadcast(owner); address newAccount = factory.createAccount(salt, 0, address(0)); + vm.assume(to != newAccount); + coolDownPeriodPassed(newAccount, lastActionTimestamp, timePassed); uint256 latestId = factory.allAccountsLength(); - vm.prank(nonOwner); - vm.expectRevert("WRONG_FROM"); - factory.transferFrom(users.accountOwner, nonOwner, latestId); + vm.prank(caller); + vm.expectRevert("NOT_AUTHORIZED"); + factory.transferFrom(owner, to, latestId); } function testFuzz_Success_STF1( - address newAccountOwner, - address nonOwner, + address owner, + address to, uint32 salt, uint32 lastActionTimestamp, uint32 timePassed - ) public notAccountOwner(newAccountOwner) notTestContracts(nonOwner) { - vm.broadcast(newAccountOwner); + ) public notTestContracts(to) { + vm.assume(owner != address(0)); + + vm.broadcast(owner); address newAccount = factory.createAccount(salt, 0, address(0)); coolDownPeriodPassed(newAccount, lastActionTimestamp, timePassed); - vm.prank(newAccountOwner); - factory.safeTransferFrom(newAccountOwner, nonOwner, newAccount); + vm.prank(owner); + factory.safeTransferFrom(owner, to, newAccount); + + assertEq(factory.ownerOfAccount(newAccount), to); } function testFuzz_Success_SFT2( - address newAccountOwner, - address nonOwner, + address owner, + address to, uint32 salt, uint32 lastActionTimestamp, uint32 timePassed - ) public notAccountOwner(newAccountOwner) notTestContracts(nonOwner) { - vm.broadcast(newAccountOwner); + ) public notTestContracts(to) { + vm.assume(owner != address(0)); + + vm.broadcast(owner); address newAccount = factory.createAccount(salt, 0, address(0)); + vm.assume(to != newAccount); + coolDownPeriodPassed(newAccount, lastActionTimestamp, timePassed); - vm.prank(newAccountOwner); - factory.safeTransferFrom(newAccountOwner, nonOwner, newAccount); + uint256 latestId = factory.allAccountsLength(); + vm.prank(owner); + factory.safeTransferFrom(owner, to, latestId); + + assertEq(factory.ownerOfAccount(newAccount), to); } function testFuzz_Success_SFT3( - address newAccountOwner, - address nonOwner, + address owner, + address to, uint32 salt, uint32 lastActionTimestamp, uint32 timePassed - ) public notAccountOwner(newAccountOwner) notTestContracts(nonOwner) { - vm.broadcast(newAccountOwner); + ) public notTestContracts(to) { + vm.assume(owner != address(0)); + + vm.broadcast(owner); address newAccount = factory.createAccount(salt, 0, address(0)); + vm.assume(to != newAccount); + coolDownPeriodPassed(newAccount, lastActionTimestamp, timePassed); uint256 latestId = factory.allAccountsLength(); - vm.prank(newAccountOwner); - factory.safeTransferFrom(newAccountOwner, nonOwner, latestId, ""); + vm.prank(owner); + factory.safeTransferFrom(owner, to, latestId, ""); + + assertEq(factory.ownerOfAccount(newAccount), to); } function testFuzz_Success_TransferFrom( - address newAccountOwner, - address nonOwner, + address owner, + address to, uint32 salt, uint32 lastActionTimestamp, uint32 timePassed - ) public notAccountOwner(newAccountOwner) notTestContracts(nonOwner) { - vm.broadcast(newAccountOwner); + ) public notTestContracts(to) { + vm.assume(owner != address(0)); + + vm.broadcast(owner); address newAccount = factory.createAccount(salt, 0, address(0)); + vm.assume(to != newAccount); + coolDownPeriodPassed(newAccount, lastActionTimestamp, timePassed); uint256 latestId = factory.allAccountsLength(); - vm.prank(newAccountOwner); - factory.transferFrom(newAccountOwner, nonOwner, latestId); + vm.prank(owner); + factory.transferFrom(owner, to, latestId); + + assertEq(factory.ownerOfAccount(newAccount), to); } }