PR-Security - `Factory.sol#safeTransferAccount()` function never will work, when Account itself want to transfer ownership

[sherlock-admin2]

PR-Security

medium

Factory.sol#safeTransferAccount() function never will work, when Account itself want to transfer ownership

Summary

The issue identified pertains to the ownership transfer mechanism of an Account within the Arcadia Protocol, specifically involving the Factory.sol#safeTransferAccount() function and its interaction with AccountV1.sol#transferOwnership() function. The current implementation restricts the transferOwnership() function to be callable only by the Factory contract, which may lead to a logical inconsistency or limitation for account owners wishing to transfer their account ownership directly.

Vulnerability Detail

The core of the issue lies in the intended functionality that allows an account owner to transfer ownership of their account. The expected process involves the account itself initiating the transfer by calling Factory.sol#safeTransferAccount(). For this transfer to be valid, the account's ownership must be transferred internally, necessitating a call to AccountV1.sol#transferOwnership().

However, AccountV1.sol#transferOwnership() is designed to be callable exclusively by the Factory contract. This design choice creates a scenario where the account, despite being the owner and initiator of the transfer, cannot directly invoke the necessary function to transfer ownership due to the restrictive permission set on transferOwnership().

Impact

This restriction significantly impacts the autonomy and flexibility of account owners in managing the ownership of their accounts. It introduces a dependency on the Factory contract for actions that account owners should be capable of initiating and completing independently. This not only complicates the ownership transfer process but also potentially delays or restricts the ability of account owners to manage their assets efficiently.

Code Snippet

• https://github.com/sherlock-audit/2023-12-arcadia/blob/main/accounts-v2/src/Factory.sol#L216-L248

  /**
   * @notice Function used to transfer an Account, called by the Account itself.
   * @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.
   */
  function safeTransferAccount(address to) public {
      if (to == address(0)) revert FactoryErrors.InvalidRecipient();
  
      uint256 id = accountIndex[msg.sender];
      if (id == 0) revert FactoryErrors.OnlyAccount();
  
      address from = _ownerOf[id];
  
      // Underflow of the sender's balance is impossible because we check for
      // ownership above and the recipient's balance can't realistically overflow.
      unchecked {
          _balanceOf[from]--;
          _balanceOf[to]++;
      }
  
      _ownerOf[id] = to;
  
      delete getApproved[id];
  
      if (
          to.code.length != 0
              && ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, "")
                  != ERC721TokenReceiver.onERC721Received.selector
      ) revert FactoryErrors.UnsafeRecipient();
  
      emit Transfer(from, to, id);
  }

Tool used

Manual Review

Recommendation

To resolve this issue, a more flexible permission model for the transferOwnership() function in AccountV1.sol is recommended. One approach could be to allow the account itself (i.e., the current owner) to call transferOwnership(), in addition to the Factory contract. This can be achieved by modifying the require statement to include a check that allows either the Factory or the current owner to execute the function:

require(msg.sender == factory || msg.sender == owner, "Only factory or owner can call");

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid

[nevillehuang]

Invalid, this is intended, transfers can only be performed via the factory. Accounts themselves can never (may never) be transferred by themselves without getting processed in the factory.