Nyxaris - H-1 Lack of Ownership Verification which leads vulnerability to gain control over accounts

[sherlock-admin2]

Nyxaris

high

H-1 Lack of Ownership Verification which leads vulnerability to gain control over accounts

Summary

The absence of ownership verification in the safeTransferFrom function increases the likelihood of unauthorized transfers, empowering malicious actors to exploit the system. This vulnerability heightens the risk of account takeovers, fund misappropriation, and compromised data integrity, significantly impacting system security.

Vulnerability Detail

The original implementation of the function lacked essential checks to verify if the msg.sender was the owner of the account being transferred. This omission could potentially allow unauthorized transfers of accounts, leading to security vulnerabilities.

Impact

Lack of ownership verification exposes the system to unauthorized account transfers, creating a critical vulnerability. Malicious actors could exploit this loophole to wrest control over accounts they do not rightfully own. This compromises the system's integrity and security, potentially leading to financial losses, unauthorized access to sensitive information, and erosion of user trust. Without stringent ownership checks, the system becomes susceptible to exploitation, posing significant risks to its overall stability and reliability.

Code Snippet

  /**
     * @notice Function used to transfer an Account between users based on Account id.
     * @param from The sender.
     * @param to The target.
     * @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.
     */
    function safeTransferFrom(address from, address to, uint256 id) public override {
// @audit no check for the msg.sender owns the account to be transferred.
        IAccount(allAccounts[id - 1]).transferOwnership(to);
        super.safeTransferFrom(from, to, id);
    }

Tool used

Manual Review

Recommendation

There are recommendations to consider: 1.) Adding a check to ensure that the account being transferred exists. 2.) Adding a check to ensure that the msg.sender is the same as the from address. 3.) Adding a check to ensure that the msg.sender is the current owner of the account being transferred. These additional checks help to ensure that the transfer is valid and authorized. Code

Summary

The absence of ownership verification in the safeTransferFrom function increases the likelihood of unauthorized transfers, empowering malicious actors to exploit the system. This vulnerability heightens the risk of account takeovers, fund misappropriation, and compromised data integrity, significantly impacting system security.

Vulnerability Detail

The original implementation of the function lacked essential checks to verify if the msg.sender was the owner of the account being transferred. This omission could potentially allow unauthorized transfers of accounts, leading to security vulnerabilities.

Impact

Lack of ownership verification exposes the system to unauthorized account transfers, creating a critical vulnerability. Malicious actors could exploit this loophole to wrest control over accounts they do not rightfully own. This compromises the system's integrity and security, potentially leading to financial losses, unauthorized access to sensitive information, and erosion of user trust. Without stringent ownership checks, the system becomes susceptible to exploitation, posing significant risks to its overall stability and reliability.

Code Snippet

  /**
     * @notice Function used to transfer an Account between users based on Account id.
     * @param from The sender.
     * @param to The target.
     * @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.
     */
    function safeTransferFrom(address from, address to, uint256 id) public override {
// @audit no check for the msg.sender owns the account to be transferred.
        IAccount(allAccounts[id - 1]).transferOwnership(to);
        super.safeTransferFrom(from, to, id);
    }

Tool used

Manual Review

Recommendation

There are recommendations to consider: 1.) Adding a check to ensure that the account being transferred exists. 2.) Adding a check to ensure that the msg.sender is the same as the from address. 3.) Adding a check to ensure that the msg.sender is the current owner of the account being transferred. These additional checks help to ensure that the transfer is valid and authorized. Coder

/**
 * @notice Function used to transfer an Account between users based on Account address.
 * @param from The sender.
 * @param to The target.
 * @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.
 */
function safeTransferFrom(address from, address to, address account) public {
+    require (ownerOfAccount(allAccounts[id - 1])==msg.sender," Sender must be owner the account");
+   require(accountIndex[allAccounts[id - 1]] != 0, "Account does not exist");
+    require(from == msg.sender, "Sender must be from");

    uint256 id = accountIndex[account];
    IAccount(allAccounts[id - 1]).transferOwnership(to);
    super.safeTransferFrom(from, to, id);
}

[nevillehuang]

Invalid, relevant access controls implemented in ERC721.sol as seen here