sherlock-audit / 2023-12-arcadia-judging

19 stars 15 forks source link

Nyxaris - H-0 Lack of Ownership Verification which leads vulnerability to gain control over accounts #132

Closed sherlock-admin closed 9 months ago

sherlock-admin commented 9 months ago

Nyxaris

high

H-0 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

  function safeTransferFrom(address from, address to, address account) public {
// @audit no check for the msg.sender owns the account.
        uint256 id = accountIndex[account];
        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

/**
 * @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(account)==msg.sender," Sender must be owner the account");
+   require(accountIndex[account] != 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);
}

Duplicate of #133

sherlock-admin2 commented 9 months ago

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

takarez commented:

invalid