Risk of malicious mass transferCurvesToken operations preventing users from successfully purchasing new curvesTokenSubjects

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L318
https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L278
https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L330

Vulnerability details

Impact

In the Curves contract, when a user purchases or creates a CurvesToken, they can transfer it to a specified address via the transferCurvesToken function. When the from and to addresses are different, the protocol will add the curvesTokenSubject to the to address's ownedCurvesTokenSubjects list via _addOwnedCurvesTokenSubject.

When a user purchases a new curvesTokenSubject via buyCurvesToken, the protocol will also add the curvesTokenSubject to the user's ownedCurvesTokenSubjects list via _addOwnedCurvesTokenSubject. Unfortunately, _addOwnedCurvesTokenSubject uses a for loop to check if curvesTokenSubject already exists in ownedCurvesTokenSubjects. Therefore, if the ownedCurvesTokenSubjects list is too large, it can cause out-of-gas failures for purchases.

An adversary can exploit this by creating a large number of useless CurvesTokens and batch-sending them to Curves platform users. This results in the users having enormous ownedCurvesTokenSubjects lists, and the protocol does not have a way to remove data from ownedCurvesTokenSubjects. It is finally leading to all Curves platform users being unable to purchase new CurvesTokens.

It is clear that competitors of the Curves platform may exploit this vulnerability to disrupt it, in order to undermine a competitor.

The core issue is that the protocol does not have the functionality to remove data from the ownedCurvesTokenSubjects list. It is not fundamentally caused by the DoS risk from the for loop in _addOwnedCurvesTokenSubject.

Without the ability to remove ownedCurvesTokenSubjects, users have no way to clean up the list if it becomes cluttered with unused entries. This leaves the list vulnerable to uncontrolled growth that could block adding new subjects. The for loop is just a symptom of this underlying limitation. The key is implementing subject removal to enable better list management.

Proof of Concept

First, a malicious user can continuously use different curvesTokenSubjects to call the Curves protocol's buyCurvesToken function to create a large number of CurvesTokens.
https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L211-L216
https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L263-L280

function buyCurvesToken(address curvesTokenSubject, uint256 amount) public payable {
    uint256 startTime = presalesMeta[curvesTokenSubject].startTime;
    if (startTime != 0 && startTime >= block.timestamp) revert SaleNotOpen();

    _buyCurvesToken(curvesTokenSubject, amount);
}

function _buyCurvesToken(address curvesTokenSubject, uint256 amount) internal {
    uint256 supply = curvesTokenSupply[curvesTokenSubject];
    if (!(supply > 0 || curvesTokenSubject == msg.sender)) revert UnauthorizedCurvesTokenSubject();

    uint256 price = getPrice(supply, amount);
    (, , , , uint256 totalFee) = getFees(price);

    if (msg.value < price + totalFee) revert InsufficientPayment();

    curvesTokenBalance[curvesTokenSubject][msg.sender] += amount;
    curvesTokenSupply[curvesTokenSubject] = supply + amount;
    _transferFees(curvesTokenSubject, true, price, amount, supply);

    // If is the first token bought, add to the list of owned tokens
    if (curvesTokenBalance[curvesTokenSubject][msg.sender] - amount == 0) {
        _addOwnedCurvesTokenSubject(msg.sender, curvesTokenSubject);
    }
}

Then they can transfer a large number of different CurvesTokens to other participants on the Curves platform using the transferCurvesToken function. The curvesTokenSubject will be added to these participants' ownedCurvesTokenSubjects lists via _addOwnedCurvesTokenSubject. When the ownedCurvesTokenSubjects list is large enough, the protocol will be unable to perform _addOwnedCurvesTokenSubject for these participants.
https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L296-L299
https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L313-L325

function transferCurvesToken(address curvesTokenSubject, address to, uint256 amount) external {
    if (to == address(this)) revert ContractCannotReceiveTransfer();
    _transfer(curvesTokenSubject, msg.sender, to, amount);
}

function _transfer(address curvesTokenSubject, address from, address to, uint256 amount) internal {
    if (amount > curvesTokenBalance[curvesTokenSubject][from]) revert InsufficientBalance();

    // If transferring from oneself, skip adding to the list
    if (from != to) {
        _addOwnedCurvesTokenSubject(to, curvesTokenSubject);
    }

    curvesTokenBalance[curvesTokenSubject][from] = curvesTokenBalance[curvesTokenSubject][from] - amount;
    curvesTokenBalance[curvesTokenSubject][to] = curvesTokenBalance[curvesTokenSubject][to] + amount;

    emit Transfer(curvesTokenSubject, from, to, amount);
}

Therefore, when these users try to purchase new CurvesTokens again via buyCurvesToken, since _addOwnedCurvesTokenSubject will run out of gas, they will never be able to buy new CurvesTokens.
https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L278
https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L330

function _buyCurvesToken(address curvesTokenSubject, uint256 amount) internal {
    ...
    if (curvesTokenBalance[curvesTokenSubject][msg.sender] - amount == 0) {
        _addOwnedCurvesTokenSubject(msg.sender, curvesTokenSubject);
    }
}

function _addOwnedCurvesTokenSubject(address owner_, address curvesTokenSubject) internal {
    address[] storage subjects = ownedCurvesTokenSubjects[owner_];
    for (uint256 i = 0; i < subjects.length; i++) {
        if (subjects[i] == curvesTokenSubject) {
            return;
        }
    }
    subjects.push(curvesTokenSubject);
}

Tools Used

N/A

Recommended Mitigation Steps

It is recommended to add the ability to remove a specified curvesTokenSubject from the ownedCurvesTokenSubjects list.

There are two viable solutions:

1. Separately add a publicly callable removeOwnedCurvesTokenSubject function, so users can remove useless curvesTokenSubjects from the list.

2. In the _transfer function, add _removeOwnedCurvesTokenSubject logic for the from address. When the share is transferred and curvesTokenBalance[curvesTokenSubject][from] is 0, remove the curvesTokenSubject via _removeOwnedCurvesTokenSubject. Users can use this to simultaneously transfer useless curvesTokenSubjects to other addresses while updating their own ownedCurvesTokenSubjects list. As follows:
   https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L313

function _transfer(address curvesTokenSubject, address from, address to, uint256 amount) internal {
    if (amount > curvesTokenBalance[curvesTokenSubject][from]) revert InsufficientBalance();

    // If transferring from oneself, skip adding to the list
    if (from != to) {
        _addOwnedCurvesTokenSubject(to, curvesTokenSubject);
    }

    curvesTokenBalance[curvesTokenSubject][from] = curvesTokenBalance[curvesTokenSubject][from] - amount;
    curvesTokenBalance[curvesTokenSubject][to] = curvesTokenBalance[curvesTokenSubject][to] + amount;

    if (curvesTokenBalance[curvesTokenSubject][from] == 0) {
        _removeOwnedCurvesTokenSubject(from, curvesTokenSubject);
    }

    emit Transfer(curvesTokenSubject, from, to, amount);
}

Assessed type

DoS

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #72

[c4-pre-sort]

raymondfam marked the issue as duplicate of #1116

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[c4-judge]

alcueca marked the issue as partial-75

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca changed the severity to 3 (High Risk)