Whitelised accounts can be forcefully DoSed from buying curveTokens during the presale

criticalCode4rena

Lines of code

https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L328-L336 https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L276-L279

Vulnerability details

Impact

Whitelisted accounts can be DoSed from buying curveTokens during the presale by a malicious party, as a result, the user who owns the whitelisted account will be forced to miss the presale and it will be able to acquire the curveTokens only during the open sale using a different account.

Proof of Concept

When a whitelised account purchases curveTokens during the presale, the user calls the Curves::buyCurvesTokenWhitelisted() function, internally, this function verifies the provided proof and if valid, calls the Curves::_buyCurvesToken() function where if the account is purchasing curveTokens of the curveTokenSubject for the first time (which it does, because its his first purchase during a presale), it calls the Curves::_addOwnedCurvesTokenSubject() function, and in this function, it will load all the curveTokens owned by the account ownedCurvesTokenSubjects, then it will iterate over this array, if the address of the curvesTokenSubject is already in the user's array (which is not, because the account is buying curveTokens of this curvesTokenSubject for the first time), the function just returns, if the curvesTokenSubject is not in the array, then the curvesTokenSubject is added to the array.

• One of the problems that causes malicious parties to DoS whitelisted accounts from buying during a presale it is the fact that the ownedCurvesTokenSubjects is loaded from storage, and the variable that is iterated on the for loop inside the Curves::_addOwnedCurvesTokenSubject() function is loaded from storage, thus, it will consume more gas, thus, less iteration will be able to do.

Now, not only because the for loop is iterating a storage variable automatically means this is a problem, the second problem is that this array only grows, even though the account dumps/sells/transfers/withdraw curveTokens from different curveTokenSubjects, the array ownedCurvesTokenSubjects never decreases, once it has added a registry, it will be there forever. The only way to decrease the length of an array is by poping values from it, but, right now, it is not implemented anywhere that if the user stops owning curveTokens of a curveTokenSubject the address of the curveTokenSubject is poped from the ownedCurvesTokenSubjects array.

Finally, the third problem is that the ownedCurvesTokenSubjects array can be manipulated at will by external parties, not only by the account itself. When any account does a transfer of curveTokens to another account, either by calling the Curves::transferCurvesToken() function or the Curves::transferAllCurvesTokens() function, any of the two functions will internally call the Curves::_transfer() function, which it will call the Curves::_addOwnedCurvesTokenSubject() function and update the ownedCurvesTokenSubjects array of the to address.

• For the purpose of this attack, the to address would be the address of a whitelisted account, which means, any user can transfer curveTokens of a nobody subjectToken to a whitelisted account for a presale of a curveTokenSubject, this will cause the ownedCurvesTokenSubjects array of the whitelisted account to grow and grow, up until the point that causes a gas error because of the length of the array.

Let's do a walkthrough of the code and see in detail everything that was mentioned previously.

Curves.sol

solidity📋
contract Curves is CurvesErrors, Security {
  ...
  function _buyCurvesToken(address curvesTokenSubject, uint256 amount) internal {
    ...
    ...
    ...

    //@audit-info => If is the first time the account is purchasing the curvesTokens of the curvesTokenSubject, will call the _addOwnedCurvesTokenSubject()
    //@audit-info => When a whitelisted account is purchasing tokens during the presale, the account owns 0 curveTokens of the curvesTokenSubject who launches the presale, therefore, the _addOwnedCurvesTokenSubject() is called

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

  function transferCurvesToken(address curvesTokenSubject, address to, uint256 amount) external {
    ...
    _transfer(curvesTokenSubject, msg.sender, to, amount);
  }

  function transferAllCurvesTokens(address to) external {
    ...
        ...
            _transfer(subjects[i], msg.sender, to, amount);
        ...
    ...
  }

  function _transfer(address curvesTokenSubject, address from, address to, uint256 amount) internal {
        ...

        // If transferring from oneself, skip adding to the list
        if (from != to) {
            //@audit-issue => When transferring curveTokens from one account to another, the _addOwnedCurvesTokenSubject() function is called
            //@audit-issue => It means, anybody can transfer curveTokens of a nobody subjectToken to a whitelisted account to inflate his `ownedCurvesTokenSubjects` array till the point that the iteration causes a gas error and reverts the tx!!! 
            _addOwnedCurvesTokenSubject(to, curvesTokenSubject);
        }
        ...
  }

  function _addOwnedCurvesTokenSubject(address owner_, address curvesTokenSubject) internal {
    //@audit-issue => Issue 1: iterates over a variable that is reading from storage!
    address[] storage subjects = ownedCurvesTokenSubjects[owner_];
    for (uint256 i = 0; i < subjects.length; i++) {
        if (subjects[i] == curvesTokenSubject) {
            return;
        }
    }
    //@audit-issue => Issue 3: When a malicious party transfers curvesTokens of a nobody subjectToken to a whitelisted account, the address of the nobody curvesTokenSubject is added to the `ownedCurvesTokenSubjects` array of the whitelisted account.
    subjects.push(curvesTokenSubject);
  }
  
  ...
  ...
  ...
  
  //@audit-issue => Issue 2: Anywhere in the Curves contract is an implementation that allows accounts to pop curvesTokenSubject addresses from their `ownedCurvesTokenSubjects` array
  //@audit-info => By poping addresses from the `ownedCurvesTokenSubjects` array would be the only way that a user could prevent the permanent DoS and be able to purchase tokens using the whitelisted account during the presale!
}

Now that we've seen where and why the permanent DoS on whitelisted accounts can be performed, let's see the attack vector.

• A subjectToken launches a presale and whitelists X number of accounts to allow them to participate in his presale. A malicious user creates new curvesTokens using random accounts, then, using a single account buys 1 curveTokens of all the random tokenSubjects, then, proceeds to call the Curves::transferAllCurvesTokens() function, as a result of this, in a single call, the malicious user will inflate the ownedCurvesTokenSubjects array of the whitelisted account, this will cause that when the whitelisted account attempts to purchase the curvesTokens of the real subjectToken during the presale, his transaction will be reverted because his ownedCurvesTokenSubjects array was filled with registries of nobodies subjectTokens, the array was inflated so much till the point that it can't be iterated and will fail with a gas exception. The whitelisted account has no means to pop any of those registries from his ownedCurvesTokenSubjects array, and as a result of this, the user who owns the whitelisted account was forcefully dosed during the presale, now, the only way for him to acquire curveTokens of that tokenSubject will be by using a different account during the open-sale.

Tools Used

Manual Audit

Recommended Mitigation Steps

The most straightforward mitigation to prevent the permanent DoS is to implement logic that allows accounts to get rid of curveTokens from tokenSubjects they don't want to own.

• Make sure to implement the pop() functionality to the ownedCurvesTokenSubjects array when the account doesn't own any curveToken of a tokenSubject.

  • This should be implemented in the functions Curves::sellCurvesToken() function & Curves::_transfer() function. Whenever the account's curvesTokenBalance is updated on any of these two functions, make sure to validate if the post balance is 0, if so, pop the subjectToken's address from the ownedCurvesTokenSubjects array of the account.

• By allowing users to have control over their ownedCurvesTokenSubjects array, there won't be any incentive from third parties to attempt to cause a DoS by inflating the users' ownedCurvesTokenSubjects array, now, each user will be able to clean up their array as they please.

• A more elaborated mitigation that will require more changes across the codebase is to use EnumerableSets instead of arrays, and make sure to implement correctly the functions offered by the EnumerableSets, such as .contain() .delete() .add()

  • But in the end, the objective must be the same, allow users to have control over their ownedCurvesTokenSubjects, if they stop owning a curveToken of a certain tokenSubject, remove that address from the ownedCurvesTokenSubjects variable.

Assessed type

DoS