Lines of code

https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/Cred.sol#L695-L729

Vulnerability details

Impact

Permanently locked funds, DoS on share selling, tampering internal accounting.

Proof of Concept

The _removeCredIdPerAddress function is used when updating the curators' mapping of _credIdsPerAddress when buying/selling shares of a cred. It is a critical function as it basically updates the system to let it know a user has gotten rid of shares in a certain credId. The issue is that it's left as public, so anyone can wipe the entries for an address leading to the users' funds being stuck since when they try to sell their shares it will revert.

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

    // 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_]--;
    }
}

Once a user has purchased shares for a given cred, anyone can delete their _credIdsPerAddress until they reach 0 and causing reverts in credIdsPerAddress[sender_].length == 0 check whenever they try to sell them and retrieve their funds.

Tools Used

Manual review

Recommended Mitigation Steps

Fix visibility to internal.

Assessed type

DoS

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

Anyone can delete curators' creds, reverting any attempts to sell shares #172