Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🔒️ prevent accounts from owning themselves #171

Merged
merged 2 commits into from
Mar 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions src/Factory.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
}

Expand All @@ -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);
}

Expand All @@ -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);
}

Expand All @@ -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();
Expand Down
10 changes: 9 additions & 1 deletion test/fuzz/Factory/SafeTransferAccount.fuzz.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down
Loading
Loading