Lines of code

https://github.com/code-423n4/2024-08-phi/blob/main/src/Cred.sol#L685 https://github.com/code-423n4/2024-08-phi/blob/main/src/Cred.sol#L695

Vulnerability details

Impact

In the Cred contract, the _addCredIdPerAddress and _removeCredIdPerAddress methods are used in the Cred contract to manage the list of Cred IDs held by the user. These two methods are triggered when the user buys or sells Cred by calling the _updateCuratorShareBalance method to update the user's Cred share.

The _addCredIdPerAddress and _removeCredIdPerAddress methods in the Cred contract are publicly accessible, which poses a significant security risk. These methods are responsible for managing the list of Cred IDs that a particular user owns. If these methods are called by unauthorized parties, it could lead to an incorrect state within the contract. For instance, an attacker could add or remove Cred IDs for any user, which would disrupt the integrity of the data, potentially leading to issues such as:

Inaccurate Record Keeping: Unauthorized users could manipulate the records of other users, adding or removing Cred IDs from their ownership, leading to inconsistencies in the contract's state.
Financial Exploits: If an attacker removes a Cred ID from a user before a transaction, it could prevent the user from selling or interacting with their assets correctly, potentially causing financial loss or preventing legitimate actions.
Denial of Service: By continuously adding or removing Cred IDs from users' lists, an attacker could flood the system with unnecessary state changes, leading to increased gas costs and potential denial of service.

Proof of Concept

https://github.com/code-423n4/2024-08-phi/blob/main/src/Cred.sol#L685

、The following code snippets from the Cred contract illustrate the potential vulnerability:

    // Function to add a new credId to the address's list

    function _addCredIdPerAddress(uint256 credId_, address sender_) public {
        // Add the new credId to the array 
        _credIdsPerAddress[sender_].push(credId_);
        // Store the index of the new credId 
        _credIdsPerAddressCredIdIndex[sender_][credId_] = _credIdsPerAddressArrLength[sender_];
        // Increment the array length counter 
        _credIdsPerAddressArrLength[sender_]++;
    }

    // Function to remove a credId from the address's list

    function _removeCredIdPerAddress(uint256 credId_, address sender_) public {
        // Check if the array is empty
        if (_credIdsPerAddress[sender_].length == 0) revert EmptyArray();

        // Get the index of the credId to remove
        uint256 indexToRemove = _credIdsPerAddressCredIdIndex[sender_][credId_];
        // Check if the index is valid 
        if (indexToRemove >= _credIdsPerAddress[sender_].length) revert IndexOutofBounds();

        // Verify that the credId at the index matches the one we want to remove
        uint256 credIdToRemove = _credIdsPerAddress[sender_][indexToRemove]; 
        if (credId_ != credIdToRemove) revert WrongCredId();

        // Get the last element in the array
        uint256 lastIndex = _credIdsPerAddress[sender_].length - 1;
        uint256 lastCredId = _credIdsPerAddress[sender_][lastIndex];
        // Move the last element to the position of the element we're removing
        _credIdsPerAddress[sender_][indexToRemove] = lastCredId;

        // Update the index of the moved element, if it's not the one we're removing
        if (indexToRemove < lastIndex) {
            _credIdsPerAddressCredIdIndex[sender_][lastCredId] = indexToRemove;
        }

        // Remove the last element (which is now a duplicate)
        _credIdsPerAddress[sender_].pop();

        // Remove the index mapping for the removed credId
        delete _credIdsPerAddressCredIdIndex[sender_][credIdToRemove];

        // Decrement the array length counter, if it's greater than 0 
        if (_credIdsPerAddressArrLength[sender_] > 0) {
            _credIdsPerAddressArrLength[sender_]--;
        }
    }

As these methods are marked as public, they can be called by any external account. This lack of access control opens the door for potential abuse.

Tools Used

VSCode

Recommended Mitigation Steps

Restrict Access: Change the visibility of _addCredIdPerAddress and _removeCredIdPerAddress from public to internal or private to ensure they are only callable by the contract’s internal methods.
Implement Access Control: If public access is necessary, implement role-based access control (RBAC) using libraries like OpenZeppelin's AccessControl to ensure that only authorized roles (e.g., contract owner or specific smart contracts) can invoke these methods.

Assessed type

Access Control

code-423n4 / 2024-08-phi-findings

Unrestricted Cred ID Manipulation Vulnerability #121