| title | sponsor | slug | date | findings | contest |
|---|---|---|---|---|---|
Caviar contest |
Caviar |
2022-12-caviar |
2023-01-26 |
193 |
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit contest 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 contest outlined in this document, C4 conducted an analysis of the Caviar smart contract system written in Solidity. The audit contest took place between December 12—December 19 2022.
130 Wardens contributed reports to the Caviar contest:
- 0x1f8b
- 0x52
- 0xAgro
- 0xDave
- 0xDecorativePineapple
- 0xGusMcCrae
- 0xSmartContract
- 0xab00
- 0xhacksmithh
- 0xmuxyz
- 0xxm
- 8olidity
- 9svR6w
- Apocalypto (cRat1st0s, reassor, and M0ndoHEHE)
- Aymen0909
- BAHOZ
- BPZ (pa6221, Bitcoinfever244, and PrasadLak)
- Bnke0x0
- Bobface
- Breeje
- CRYP70
- Chom
- Diana
- ElKu
- Franfran
- HE1M
- HardlyCodeMan
- IllIllI
- JC
- Janio
- Jeiwan
- JrNet
- Junnon
- KingNFT
- Koolex
- Lambda
- Madalad
- NoamYakov
- RaymondFam
- ReyAdmirado
- Rolezn
- SamGMK
- SleepingBugs (Deivitto and 0xLovesleep)
- Tointer
- Tricko
- UNCHAIN (Tomo, mashharuki, yawn, keit, ahayashi, sho, KazumaHamamoto, cardene, cotoneum, mugi, yosuke, masaru, kevin_katsu, junya, daikai, mabuk, mameta, kyok1st, 0xShin, hamaup, kii, and yoki)
- UdarTeam (ahmedov and tourist)
- Zarf
- __141345__
- adriro
- ahayashi
- ak1
- aviggiano
- bytehat
- c3phas
- carlitox477
- carrotsmuggler
- caventa
- cccz
- chaduke
- cozzetti
- cryptonue
- cryptostellar5
- dic0de
- dipp
- eyexploit
- fs0c
- gz627
- gzeon
- h0wl
- haku
- hansfriese
- helios
- hihen
- imare
- immeas
- izhelyazkov
- kiki_dev
- koxuan
- ktg
- ladboy233
- lukris02
- lumoswiz
- mauricio1802
- millersplanet
- minhquanym
- minhtrng
- nicobevi
- obront
- oyc_109
- pavankv
- rajatbeladiya
- ret2basic
- rjs
- rvierdiiev
- saneryee
- seyni
- shung
- supernova
- tnevler
- unforgiven
- wait
- yixxas
This contest was judged by berndartmueller.
Final report assembled by liveactionllama.
The C4 analysis yielded an aggregated total of 8 unique vulnerabilities. Of these vulnerabilities, 3 received a risk rating in the category of HIGH severity and 5 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 25 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 31 reports recommending gas optimizations.
All of the issues presented here are linked back to their original finding.
The code under review can be found within the C4 Caviar contest repository, and is composed of 4 smart contracts written in the Solidity programming language and includes 318 lines of Solidity code.
C4 assesses the severity of disclosed vulnerabilities according to a methodology based on OWASP standards.
Vulnerabilities are divided into 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
Further information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website.
Submitted by carlitox477, also found by minhquanym, gzeon, 9svR6w, Lambda, koxuan, KingNFT, cozzetti, rvierdiiev, and cccz
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L95
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L137
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L172
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L203
Current implementation of functions add, remove, buy and sell first transfer fractional tokens, and then base tokens.
If this base token is ERC777 (extension of ERC20), we can call this function without updating the base token balance, but updating the fractional token balance.
Allows to drain funds of a pairs which implements an ERC-777 token.
function buy(uint256 outputAmount, uint256 maxInputAmount) public payable returns (uint256 inputAmount) {
// *** Checks *** //
// check that correct eth input was sent - if the baseToken equals address(0) then native ETH is used
require(baseToken == address(0) ? msg.value == maxInputAmount : msg.value == 0, "Invalid ether input");
// calculate required input amount using xyk invariant
+ @audit Use current balances
inputAmount = buyQuote(outputAmount);
// check that the required amount of base tokens is less than the max amount
require(inputAmount <= maxInputAmount, "Slippage: amount in");
// *** Effects *** //
+ @audit Modifies just fractional balance
// transfer fractional tokens to sender
_transferFrom(address(this), msg.sender, outputAmount);
// *** Interactions *** //
if (baseToken == address(0)) {
// refund surplus eth
uint256 refundAmount = maxInputAmount - inputAmount;
if (refundAmount > 0) msg.sender.safeTransferETH(refundAmount);
} else {
// transfer base tokens in
+ @audit If an ERC-777 token is used, we can re call buy function with the same balance of base token, but with different fractional balance
ERC20(baseToken).safeTransferFrom(msg.sender, address(this), inputAmount);
}
emit Buy(inputAmount, outputAmount);
}function buyQuote(uint256 outputAmount) public view returns (uint256) {
return (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997);
}The buy quote is used to calculate the amount of fractional token that the user will receive, and it should be less/equal to maxInputAmount sent by parameter in order to achieve a successful execution of function buy.
Current buy quote can be mathematically expressed as:
Then, about sales
function sell(uint256 inputAmount, uint256 minOutputAmount) public returns (uint256 outputAmount) {
// *** Checks *** //
// calculate output amount using xyk invariant
outputAmount = sellQuote(inputAmount);
// check that the outputted amount of fractional tokens is greater than the min amount
require(outputAmount >= minOutputAmount, "Slippage: amount out");
// *** Effects *** //
// transfer fractional tokens from sender
+ //@audit fractional balance is updated
_transferFrom(msg.sender, address(this), inputAmount);
// *** Interactions *** //
if (baseToken == address(0)) {
// transfer ether out
msg.sender.safeTransferETH(outputAmount);
} else {
// transfer base tokens out
+ @audit If an ERC-777 token is used, we can re call sell function with the same balance of base token, but with different fractional balance.
ERC20(baseToken).safeTransfer(msg.sender, outputAmount);
}
emit Sell(inputAmount, outputAmount);
} uint256 inputAmountWithFee = inputAmount * 997;
return (inputAmountWithFee * baseTokenReserves()) / ((fractionalTokenReserves() * 1000) + inputAmountWithFee);
}
Current sellQuote function can be expressed mathematically as:
Then we can think next scenario to drain a pair which use an ERC-777 token as base token:
- Let's suppose the pair has 1000 base tokens(BT777) and 1000 Fractional reserve tokens (FRT)
- The attacker call buy function, all with next inputs:
- outputAmount = 50
- maxInputAmount = 80
- The attacker implements a hook, that will be executed 6 times (using a counter inside a malicus contract) when a transfer is done, and call the buy function. After this 6 times the malicious contract is call again, but this times calls the sell function, doing a huge sell for the fractional reserve token obtained.
A simulation of this attack can be visualized in next table
| Operation | outputAmount (FRT) | maxInputAmount (BT777) | BT777 reserve | FRT reserve | inputAmount (BT777 to pay) | inputAmount < maxInputAmount |
|---|---|---|---|---|---|---|
| Attaker buy 1 | 50 | 80 | 1000 | 1000 | 52 | TRUE |
| Callback buy 2 | 50 | 80 | 1000 | 950 | 55 | TRUE |
| Callback buy 3 | 50 | 80 | 1000 | 900 | 59 | TRUE |
| Callback buy 4 | 50 | 80 | 1000 | 850 | 62 | TRUE |
| Callback buy 5 | 50 | 80 | 1000 | 800 | 66 | TRUE |
| Callback buy 6 | 50 | 80 | 1000 | 750 | 71 | TRUE |
| Callback buy 7 | 50 | 80 | 1000 | 700 | 77 | TRUE |
The result of this operation is that the attaker/malicious contract has 350 FRT, while BT777 reserve still has 1000 and FRT reserve has 650 tokens. The success execution needs that the attacker pays 442 BT777 eventually.
To do this, the last operation of the malicious contract is calling sell function
| Operation | inputAmount(BT777) | minOutputAmount | BT777 reserve | FRT reserve | outputAmount (BT777 to receive) | outputAmount > minOutputAmount |
|---|---|---|---|---|---|---|
| calback Sell | 350 | 442 | 1000 | 650 | 536 | TRUE |
The result is that the attacker now controls 536 BT777, the attacker use this balance to pay the debt of 442 BT77, with a profit of 94 BT77 tokens.
Add openzeppelin nonReentrant modifier to mentioned functions, or state clear in the documentation that this protocol should not be used with ERC777 tokens.
outdoteth (Caviar) acknowledged and commented:
Technically valid, though we don't intend to support erc777 tokens.
Submitted by Jeiwan, also found by minhtrng, minhquanym, HE1M, wait, hansfriese, BAHOZ, unforgiven, 0xxm, Junnon, bytehat, UNCHAIN, carlitox477, RaymondFam, Chom, CRYP70, 9svR6w, mauricio1802, __141345__, hihen, caventa, koxuan, obront, nicobevi, shung, cccz, Bobface, and chaduke
Liquidity providers may lose a portion of provided liquidity in either of the pair tokens. While the minLpTokenAmount protects from slippage when adding liquidity, it doesn't protect from providing liquidity at different K.
The Pair contract is designed to receive liquidity from liquidity providers (Pair.sol#L63). First liquidity provider in a pool may provide arbitrary token amounts and set the initial price (Pair.sol#L425-L426), but all other liquidity providers must provide liquidity proportionally to current pool reserves (Pair.sol#L420-L423). Since a pool is made of two tokens and liquidity is provided in both tokens, there's a possibility for a discrepancy: token amounts may be provided in different proportions. When this happens, the smaller of the proportions is chosen to calculate the amount of LP tokens minted (Pair.sol#L420-L423):
// calculate amount of lp tokens as a fraction of existing reserves
uint256 baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves();
uint256 fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves();
return Math.min(baseTokenShare, fractionalTokenShare);As a result, the difference in proportions will create an excess of tokens that won't be redeemable for the amount of LP tokens minted. The excess of tokens gets, basically, donated to the pool: it'll be shared among all liquidity providers of the pool. While the minLpTokenAmount argument of the add function (Pair.sol#L63) allows liquidity providers to set the minimal amount of LP tokens they want to receive, it doesn't allow them to minimize the disproportion of token amounts or avoid it at all.
// test/Pair/unit.Add.t.sol
function testLockOfFunds_AUDIT() public {
address alice = address(0x31337);
address bob = address(0x12345);
vm.label(alice, "alice");
vm.label(bob, "bob");
deal(address(usd), alice, 100e18, true);
deal(address(usd), bob, 100e18, true);
deal(address(p), alice, 100e18, true);
deal(address(p), bob, 100e18, true);
// Alice is the first liquidity provider.
vm.startPrank(alice);
usd.approve(address(p), type(uint256).max);
p.add(10 ether, 10 ether, 0);
vm.stopPrank();
// Bob provides liquidity to the pool and sets the minimal LP amount.
// The token amounts are deposited in different proportions, thus the smaller
// one will be chosen to calculate the amount of LP tokens Bob will receive.
vm.startPrank(bob);
usd.approve(address(p), type(uint256).max);
uint256 minLPAmount = 1e18;
uint256 bobLPAmount = p.add(1.2 ether, 1 ether, minLPAmount);
vm.stopPrank();
// Bob has received the minimal LP amount he wanted.
assertEq(bobLPAmount, minLPAmount);
// However, after removing all his liquidity from the pool...
(uint256 bobUSDBefore, uint256 bobFracBefore) = (usd.balanceOf(bob), p.balanceOf(bob));
vm.prank(bob);
p.remove(minLPAmount, 0, 0);
(uint256 bobUSDAfter, uint256 bobFracAfter) = (usd.balanceOf(bob), p.balanceOf(bob));
// ... Bob received less USD than he deposited.
assertEq(bobUSDAfter - bobUSDBefore, 1.018181818181818181 ether);
assertEq(bobFracAfter - bobFracBefore, 1.000000000000000000 ether);
}In the add function, consider calculating optimal token amounts based on the amounts specified by user, current pool reserves, and the minimal LP tokens amount specified by user. As a reference, consider this piece from the Uniswap V2 Router: UniswapV2Router02.sol#L45-L60.
outdoteth (Caviar) confirmed and commented:
Fixed in: outdoteth/caviar#2
By allowing a user to specify a
minPriceandmaxPricethat they are willing to LP at along with theminLpTokenAmountthat they would like to receive. The price calculation is based on this: https://github.com/outdoteth/caviar/blob/main/src/Pair.sol#L471
Submitted by minhquanym, also found by Apocalypto, 0xDecorativePineapple, Franfran, dipp, rjs, ak1, Tricko, Jeiwan, unforgiven, hansfriese, BAHOZ, unforgiven, bytehat, UNCHAIN, immeas, SamGMK, fs0c, Tointer, haku, Koolex, __141345__, ElKu, rajatbeladiya, hihen, izhelyazkov, KingNFT, koxuan, 0x52, carrotsmuggler, yixxas, HE1M, supernova, cozzetti, rvierdiiev, SamGMK, aviggiano, seyni, lumoswiz, ladboy233, chaduke, cccz, and eyexploit
The attack vector and impact is the same as TOB-YEARN-003, where users may not receive shares in exchange for their deposits if the total asset amount has been manipulated through a large “donation”.
In Pair.add(), the amount of LP token minted is calculated as
function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256) {
uint256 lpTokenSupply = lpToken.totalSupply();
if (lpTokenSupply > 0) {
// calculate amount of lp tokens as a fraction of existing reserves
uint256 baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves();
uint256 fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves();
return Math.min(baseTokenShare, fractionalTokenShare);
} else {
// if there is no liquidity then init
return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
}
}An attacker can exploit using these steps
- Create and add
1 wei baseToken - 1 wei quoteTokento the pair. At this moment, attacker is minted1 wei LP tokenbecausesqrt(1 * 1) = 1 - Transfer large amount of
baseTokenandquoteTokendirectly to the pair, such as1e9 baseToken - 1e9 quoteToken. Since no new LP token is minted,1 wei LP tokenworths1e9 baseToken - 1e9 quoteToken. - Normal users add liquidity to pool will receive
0LP token if they add less than1e9token because of rounding division.
baseTokenShare = (X * 1) / 1e9;
fractionalTokenShare = (Y * 1) / 1e9;- Uniswap V2 solved this problem by sending the first 1000 LP tokens to the zero address. The same can be done in this case i.e. when
lpTokenSupply == 0, send the first min liquidity LP tokens to the zero address to enable share dilution. - In
add(), ensure the number of LP tokens to be minted is non-zero:
require(lpTokenAmount != 0, "No LP minted");outdoteth (Caviar) confirmed and commented:
Fixed in: outdoteth/caviar#3
Submitted by Bobface, also found by cozzetti
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L63
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L107
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L147
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L182
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L275
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L294
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L310
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L323
The Pair contract does not allow users to submit a deadline for their action. This missing feature enables pending transactions to be maliciously executed at a later point.
AMMs should provide their users with an option to limit the execution of their pending actions, such as swaps or adding and removing liquidity. The most common solution is to include a deadline timestamp as a parameter (for example see Uniswap V2). If such an option is not present, users can unknowingly perform bad trades:
- Alice wants to swap 100 fractional NFT tokens (
fTokens) for 1 ETH and later sell the 1 ETH for 1000 DAI. She signs the transaction callingPair.sellwithinputAmount = 100 fTokensandminOutputAmount = 0.99 ETHto allow for some slippage. - The transaction is submitted to the mempool, however, Alice chose a transaction fee that is too low for miners to be interested in including her transaction in a block. The transaction stays pending in the mempool for extended periods, which could be hours, days, weeks, or even longer.
- When the average gas fee dropped far enough for Alice's transaction to become interesting again for miners to include it, her swap will be executed. In the meantime, the price of
ETHcould have drastically changed. She will still at least get0.99 ETHdue tominOutputAmount, but theDAIvalue of that output might be significantly lower. She has unknowingly performed a bad trade due to the pending transaction she forgot about.
An even worse way this issue can be maliciously exploited is through MEV:
- The swap transaction is still pending in the mempool. Average fees are still too high for miners to be interested in it. The price of
fTokenhas gone up significantly since the transaction was signed, meaning Alice would receive a lot moreETHwhen the swap is executed. But that also means that herminOutputAmountvalue is outdated and would allow for significant slippage. - A MEV bot detects the pending transaction. Since the outdated
minOutputAmountnow allows for high slippage, the bot sandwiches Alice, resulting in significant profit for the bot and significant loss for Alice.
The affected functions in Pair.sol are:
add()remove()buy()sell()nftAdd()nftRemove()nftBuy()nftSell()
Introduce a deadline parameter to the mentioned functions.
Categorizing this issue into medium versus high was not immediately obvious. I came to the conclusion that this is a high-severity issue for the following reason:
I run an arbitrage MEV bot myself, which also tracks pending transactions in the mempool, though for another reason than the one mentioned in this report. There is a significant amount of pending and even dropped transactions: over 200,000 transactions that are older than one month. These transactions do all kinds of things, from withdrawing from staking contracts to sending funds to CEXs and also performing swaps on DEXs like Uniswap. This goes to show that this issue will in fact be very real, there will be very old pending transactions wanting to perform trades without a doubt. And with the prevalence of advanced MEV bots, these transactions will be exploited as described in the second example above, leading to losses for Caviar's users.
Omitted in this case, since the exploit is solely based on the fact that there is no limit on how long a transaction is allowed to be pending, which can be clearly seen when looking at the mentioned functions.
berndartmueller (judge) decreased severity to Medium
Fixed in outdoteth/caviar#6
Add a deadline check.
Submitted by obront, also found by cryptostellar5, Tricko, CRYP70, 0xmuxyz, koxuan, 8olidity, yixxas, cozzetti, ktg, and ladboy233
The price() function is expected to return the price of one fractional tokens, represented in base tokens, to 18 decimals of precision. This is laid out clearly in the comments:
/// @notice The current price of one fractional token in base tokens with 18 decimals of precision.
/// @dev Calculated by dividing the base token reserves by the fractional token reserves.
/// @return price The price of one fractional token in base tokens * 1e18.
However, the formula incorrectly calculates the price to be represented in whatever number of decimals the base token is in. Since there are many common base tokens (such as USDC) that will have fewer than 18 decimals, this will create a large mismatch between expected prices and the prices that result from the function.
Prices are calculated with the following formula, where ONE = 1e18:
return (_baseTokenReserves() * ONE) / fractionalTokenReserves();We know that fractionalTokenReserves will always be represented in 18 decimals. This means that the ONE and the
fractionalTokenReserves will cancel each other out, and we are left with the baseTokenReserves number of decimals for the final price.
As an example:
- We have
$1000USDC in reserves, which at 6 decimals is 1e9 - We have 1000 fractional tokens in reserve, which at 18 decimals is 1e21
- The price calculation is
1e9 * 1e18 / 1e21 = 1e6 - While the value should be 1 token, the 1e6 will be interpreted as just 1/1e12 tokens if we expect the price to be in 1e18
The formula should use the decimals value of the baseToken to ensure that the decimals of the resulting price ends up with 18 decimals as expected:
return (_baseTokenReserves() * 10 ** (36 - ERC20(baseToken).decimals()) / fractionalTokenReserves();This will multiple baseTokenReserves by 1e18, and then additionally by the gap between 1e18 and its own decimals count, which will result in the correct decimals value for the outputted price.
outdoteth (Caviar) confirmed and commented:
Fixed in: outdoteth/caviar#5
Always ensure that the exponent is 18 greater than the denominator.
Submitted by Zarf, also found by minhtrng, Franfran, Apocalypto, adriro, 0xDave, wait, unforgiven, Jeiwan, hansfriese, bytehat, UNCHAIN, rajatbeladiya, CRYP70, hihen, koxuan, kiki_dev, yixxas, and chaduke
In order to guarantee the contract does not become insolvent, incoming assets should be rounded up, while outgoing assets should be rounded down.
The function buyQuote() calculates the amount of base tokens required to buy a given amount of fractional tokens. However, this function rounds down the required amount, which is in favor of the buyer (i.e. he/she has to provide less base tokens for the amount of receiving fractional tokens.
Depending on the amount of current token reserves and the amount of fractional tokens the user wishes to buy, it might be possible to receive free fractional tokens.
Assume the following reserve state:
- base token reserve: 0,1 WBTC (=
1e7) - fractional token reserve: 10.000.000 (=
1e25)
The user wishes to buy 0,9 fractional tokens (=9e17). Then, the function buyQuote() will calculate the amount of base tokens as follows:
(9e17 * 1000 * 1e7) / ((1e25 - 9e17) * 997) = 0,903
As division in Solidity will round down, the amount results in 0 amount of base tokens required (WBTC) to buy 0,9 fractional tokens.
Using the example above, 0,9 fractional tokens is a really small amount (0,1 BTC / 1e7 = +- $0,00017). Moreover, if the user keeps repeating this attack, the fractional token reserve becomes smaller, which will result in a buyQuote amount of >1, after which the tokens will not be free anymore.
Additionally, as the contract incorporates a fee of 30bps, it will likely not be insolvent. The downside would be the LP holder, which will receive a fee of less than 30bps. Hence, the impact is rated as medium.
For incoming assets, it’s recommended to round up the required amount. We could use solmate’s FixedPointMathLib library to calculate the quote and round up. This way the required amount will always at least be 1 wei:
function buyQuote(uint256 outputAmount) public view returns (uint256) {
return mulDivUp(outputAmount * 1000, baseTokenReserves(), (fractionalTokenReserves() - outputAmount) * 997);
}outdoteth (Caviar) confirmed and commented:
Fixed in: outdoteth/caviar#4
Uses muldivup from solmate to round up the calculation in buyQuote.
[M-04] It's possible to swap NFT token ids without fee and also attacker can wrap unwrap all the NFT token balance of the Pair contract and steal their air drops for those token ids
Submitted by unforgiven, also found by imare and ElKu
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L217-L243
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L248-L262
Users can wrap() their NFT tokens (which id is whitelisted) and receive 1e18 fractional token or they can pay 1e18 fractional token and unwrap NFT token. there is two issue here:
- anyone can swap their NFT token id with another NFT token id without paying any fee(both ids should be whitelisted). it's swap without fee.
- attacker can swap his NFT token(with whitelisted id) for all the NFT balance of contract and steal those NFT tokens airdrop all in one transaction.
This is wrap() and unwrap() code:
function wrap(uint256[] calldata tokenIds, bytes32[][] calldata proofs)
public
returns (uint256 fractionalTokenAmount)
{
// *** Checks *** //
// check that wrapping is not closed
require(closeTimestamp == 0, "Wrap: closed");
// check the tokens exist in the merkle root
_validateTokenIds(tokenIds, proofs);
// *** Effects *** //
// mint fractional tokens to sender
fractionalTokenAmount = tokenIds.length * ONE;
_mint(msg.sender, fractionalTokenAmount);
// *** Interactions *** //
// transfer nfts from sender
for (uint256 i = 0; i < tokenIds.length; i++) {
ERC721(nft).safeTransferFrom(msg.sender, address(this), tokenIds[i]);
}
emit Wrap(tokenIds);
}
function unwrap(uint256[] calldata tokenIds) public returns (uint256 fractionalTokenAmount) {
// *** Effects *** //
// burn fractional tokens from sender
fractionalTokenAmount = tokenIds.length * ONE;
_burn(msg.sender, fractionalTokenAmount);
// *** Interactions *** //
// transfer nfts to sender
for (uint256 i = 0; i < tokenIds.length; i++) {
ERC721(nft).safeTransferFrom(address(this), msg.sender, tokenIds[i]);
}
emit Unwrap(tokenIds);
}
As you can see it's possible to wrap one NFT token (which id is whitelisted and is in merkle tree) and unwrap another NFT token without paying fee. so Pair contract create NFT swap without fee for users but there is no fee generated for those who wrapped and put their fractional tokens as liquidity providers.
The other issue with this is that some NFT tokens air drop new NFT tokens for NFT holders by making NFT holders to call getAirdrop() function. attacker can use this swap functionality to get air drop token for all the NFT balance of the Pair contract. to steps to perform this attack:
-
if Pair contract is for NFT1 and baseToken1 and also merkle tree root hash is 0x0.
-
users deposited 100 NFT1 tokens to the Pair contract.
-
NFT1 decide to airdrop some new tokens for token holders and token holders need to call
nft.getAirDrop(id)while they own the NFT id. -
attacker would create a contract and buy one of the NFT1 tokens (attackerID1) and wrap it to receive
1e18fractional tokens and perform this steps in the contract:
4.1 loop through all the NFT tokens in the Pair contract balance and:
4.2 unwrap NFT token id=i from Pair contract by paying1e18fractional token.
4.3 callnft.getAirDrop(i)and receive the new airdrop token. (the name of the function can be other thing not exactlygetAirDrop())
4.4 wrap NFT token id=i and receive1e18fractional token. -
in the end attacker would unwrap attackerID1 token from Pair contract.
so attacker was able to receive all the air drops of the NFT tokens that were in the contract address, there could be 100 or 1000 NFT tokens in the contract address and attacker can steal their air drops in one transaction(by writing a contract). those air drops belongs to all the fractional owners and contract shouldn't allow one user to take all the air drops for himself. as airdrops are common in NFT collections so this bug is critical and would happen.
also some of the NFT tokens allows users to stake some tokens for their NFT tokens and receive rewards(for example BAYC/MAYC). if a user stakes tokens for his NFT tokens then wrap those NFT tokens then it would be possible for attacker to unwrap those tokens and steal user staked amounts. in this scenario user made a risky move and wrapped NFT tokens while they have stake but as a lot of users wants to stake for their NFTs this would make them unable to use caviar protocol.
also any other action that attacker can perform by becoming the owner of the NFT token is possible by this attack and if that action can harm the NFT token holders then attacker can harm by doing this attack and performing that action.
VIM
The real solution to prevent this attack (stealing air drops) can be hard. some of the things can be done is:
- create functionality so admin can call
getAirDrop()functions during the airdrops before attacker. - call
getAirDrop()(which admin specified) function before unwrapping tokens. - make some fee for NFT token unwrapping.
- create some lock time(some days) for each wrapped NFT that in that lock time only the one who supplied that token can unwrap it.
- create some delay for unwrapping tokens and if user wants to unwrap token he would receive it after this delay.
outdoteth (Caviar) acknowledged, but disagreed with severity
berndartmueller (judge) decreased severity to Medium
Submitted by Jeiwan, also found by BPZ, ak1, Janio, hansfriese, UNCHAIN, dic0de, and ladboy233
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L391
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L479-L480
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L384
An attacker may manipulate the price of a pair by transferring tokens directly to the pair. Since the Pair contract exposes the price function, it maybe be used as a price oracle in third-party integrations. Manipulating the price of a pair may allow an attacker to steal funds from such integrations.
The Pair contract is a pool of two tokens, a base token and a fractional token. Its main purpose is to allow users to swap the tokens at a fair price. Since the price is calculated based on the reserves of a pair, it can only be changed in two cases:
- when initial liquidity is added: the first liquidity provider sets the price of a pool (Pair.sol#L85-L97); other liquidity providers cannot change the price (Pair.sol#L421-L423);
- during trades: trading adds and removes tokens from a pool, ensuring the K constant invariant is respected (Pair.sol#L194-L204, Pair.sol#L161-L173).
However, the Pair contract calculates the price using the current token balances of the contract (Pair.sol#L379-L385, Pair.sol#L477-L481):
function baseTokenReserves() public view returns (uint256) {
return _baseTokenReserves();
}
function _baseTokenReserves() internal view returns (uint256) {
return baseToken == address(0)
? address(this).balance - msg.value // subtract the msg.value if the base token is ETH
: ERC20(baseToken).balanceOf(address(this));
}
function fractionalTokenReserves() public view returns (uint256) {
return balanceOf[address(this)];
}This allows an attacker to change the price of a pool and skip the K constant invariant check that's enforced on new liquidity (Pair.sol#L421-L423).
Consider tracking pair's reserves internally, using state variables, similarly to how Uniswap V2 does that:
uint112 private reserve0; // uses single storage slot, accessible via getReserves
uint112 private reserve1; // uses single storage slot, accessible via getReservesfunction getReserves() public view returns (uint112 _reserve0, uint112 _reserve1, uint32 _blockTimestampLast) {
_reserve0 = reserve0;
_reserve1 = reserve1;
_blockTimestampLast = blockTimestampLast;
}// update reserves and, on the first call per block, price accumulators
function _update(uint balance0, uint balance1, uint112 _reserve0, uint112 _reserve1) private {
require(balance0 <= uint112(-1) && balance1 <= uint112(-1), 'UniswapV2: OVERFLOW');
uint32 blockTimestamp = uint32(block.timestamp % 2**32);
uint32 timeElapsed = blockTimestamp - blockTimestampLast; // overflow is desired
if (timeElapsed > 0 && _reserve0 != 0 && _reserve1 != 0) {
// * never overflows, and + overflow is desired
price0CumulativeLast += uint(UQ112x112.encode(_reserve1).uqdiv(_reserve0)) * timeElapsed;
price1CumulativeLast += uint(UQ112x112.encode(_reserve0).uqdiv(_reserve1)) * timeElapsed;
}
reserve0 = uint112(balance0);
reserve1 = uint112(balance1);
blockTimestampLast = blockTimestamp;
emit Sync(reserve0, reserve1);
}minhquanym (warden) commented:
@berndartmueller - The recommendation suggested that it should follow Uniswap V2 and add internal state balance. However, Uniswap V2 also has function
sync()allowing to syncreserve0andreserve1to current token balance of contract. It means if this is an issue, it will also be an issue after UniV2 (by direct transfers and callsync()immediately). Please correct me if I missed something here https://github.com/Uniswap/v2-core/blob/ee547b17853e71ed4e0101ccfd52e70d5acded58/contracts/UniswapV2Pair.sol#L198
berndartmueller (judge) commented:
@minhquanym - The specific issue demonstrated in this submission is exposing the
Pair.pricefunction, which is easily manipulatable by direct transfers and thus vulnerable as a price oracle. Uniswap V2, in comparison, uses the concept of a cumulative price weighted by the amount of time this price existed (see https://docs.uniswap.org/contracts/v2/concepts/core-concepts/oracles for more details).
outdoteth (Caviar) acknowledged
For this contest, 25 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by 0xSmartContract received the top score from the judge.
The following wardens also submitted reports: SleepingBugs, JC, rjs, minhquanym, IllIllI, 0xGusMcCrae, h0wl, unforgiven, UNCHAIN, immeas, ahayashi, RaymondFam, caventa, 8olidity, yixxas, obront, shung, cozzetti, rvierdiiev, Bnke0x0, Rolezn, aviggiano, helios, and ladboy233.
| Number | Issues Details | Context |
|---|---|---|
| [L-01] | Missing ReEntrancy Guard to withdraw function |
1 |
| [L-02] | Use safeTransferOwnership instead of transferOwnership function |
1 |
| [L-03] | Loss of precision due to rounding | 1 |
| [L-04] | Solmate's SafeTransferLib doesn't check whether the ERC20 contract exists | 10 |
| [L-05] | Should an airdrop token arrive on the pair.sol contract, it will be stuck |
1 |
Total 5 issues
| Number | Issues Details | Context |
|---|---|---|
| [N-01] | Insufficient coverage | 1 |
| [N-02] | NatSpec comments should be increased in contracts | All Contracts |
| [N-03] | Function writing that does not comply with the Solidity Style Guide |
All Contracts |
| [N-04] | Solidity compiler optimizations can be problematic | |
| [N-05] | For modern and more readable code; update import usages | 13 |
| [N-06] | Lock pragmas to specific compiler version | 5 |
| [N-07] | Use underscores for number literals | 2 |
| [N-08] | Use of bytes.concat() instead of abi.encodePacked() |
1 |
| [N-09] | Pragma version^0.8.17 version too recent to be trusted | All Contracts |
| [N-10] | Add EIP-2981 NFT Royalty Standart Support | 1 |
| [N-11] | Showing the actual values of numbers in NatSpec comments makes checking and reading code easier | 2 |
| [N-12] | Missing Event for critical parameters init and change | 3 |
| [N-13] | Add to blacklist function | 1 |
Total 13 issues
| Number | Suggestion Details |
|---|---|
| [S-01] | Project Upgrade and Stop Scenario should be |
| [S-02] | Generate perfect code headers every time |
Total 2 suggestions
https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L359-L373
Position.sol contract has no Re-Entrancy protection in withdraw function
src/Pair.sol:
function withdraw(uint256 tokenId) public {
// check that the sender is the caviar owner
require(caviar.owner() == msg.sender, "Withdraw: not owner");
// check that the close period has been set
require(closeTimestamp != 0, "Withdraw not initiated");
// check that the close grace period has passed
require(block.timestamp >= closeTimestamp, "Not withdrawable yet");
// transfer the nft to the caviar owner
ERC721(nft).safeTransferFrom(address(this), msg.sender, tokenId);
emit Withdraw(tokenId);
}
If the mint was initiated by a contract, then the contract is checked for its ability to receive ERC721 tokens. Without reentrancy guard, onERC721Received will allow an attacker controlled contract to call the mint again, which may not be desirable to some parties, like allowing minting more than allowed.
https://www.paradigm.xyz/2021/08/the-dangers-of-surprising-code
If withdraw is msg.sender contract, it can do re-entrancy by overriding onERC721Received function, it doesn't seem to be a serious problem since it conforms to check-effect-interaction pattern, but this is a clear re-entry due to access to other functions and pre-emit processing. is the entracy
reentrancy.sol:
function onERC721Received(
address,
address,
uint256,
bytes memory
) public virtual override returns (bytes4) {
//...do something
}
return this.onERC721Received.selector;
}
Use Openzeppelin or Solmate Re-Entrancy pattern.
Here is a example of a re-entrancy guard
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;
contract ReEntrancyGuard {
bool internal locked;
modifier noReentrant() {
require(!locked, "No re-entrancy");
locked = true;
_;
locked = false;
}
}Context:
2 results - 2 files
src/Caviar.sol:
4: import "solmate/auth/Owned.sol";
12: contract Caviar is Owned {
src/LpToken.sol:
4: import "solmate/auth/Owned.sol";
11: contract LpToken is Owned, ERC20 {
Description:
transferOwnership function is used to change Ownership from Owned.sol.
Use a 2 structure transferOwnership which is safer.
safeTransferOwnership, use it is more secure due to 2-stage ownership transfer.
Recommendation:
Use Ownable2Step.sol
Ownable2Step.sol
Add scalars so roundings are negligible
src/Pair.sol:
390: function price() public view returns (uint256) {
391: return (_baseTokenReserves() * ONE) / fractionalTokenReserves();
392: }
Solmate's SafeTransferLib, which is often used to interact with non-compliant/unsafe ERC20 tokens, does not check whether the ERC20 contract exists. The following code will not revert in case the token doesn't exist (yet).
This is stated in the Solmate library: https://github.com/transmissions11/solmate/blob/main/src/utils/SafeTransferLib.sol#L9
10 results - 1 file
src/Pair.sol:
94 // transfer base tokens in
95: ERC20(baseToken).safeTransferFrom(msg.sender, address(this), baseTokenAmount);
96 }
133 // if base token is native ETH then send ether to sender
134: msg.sender.safeTransferETH(baseTokenOutputAmount);
135 } else {
136 // transfer base tokens to sender
137: ERC20(baseToken).safeTransfer(msg.sender, baseTokenOutputAmount);
138 }
168 uint256 refundAmount = maxInputAmount - inputAmount;
169: if (refundAmount > 0) msg.sender.safeTransferETH(refundAmount);
170 } else {
171 // transfer base tokens in
172: ERC20(baseToken).safeTransferFrom(msg.sender, address(this), inputAmount);
173 }
199 // transfer ether out
200: msg.sender.safeTransferETH(outputAmount);
201 } else {
202 // transfer base tokens out
203: ERC20(baseToken).safeTransfer(msg.sender, outputAmount);
204 }
238 for (uint256 i = 0; i < tokenIds.length; i++) {
239: ERC721(nft).safeTransferFrom(msg.sender, address(this), tokenIds[i]);
240 }
258 for (uint256 i = 0; i < tokenIds.length; i++) {
259: ERC721(nft).safeTransferFrom(address(this), msg.sender, tokenIds[i]);
260 }
369 // transfer the nft to the caviar owner
370: ERC721(nft).safeTransferFrom(address(this), msg.sender, tokenId);
371 Add a contract exist control in functions;
pragma solidity >=0.8.0;
function isContract(address _addr) private returns (bool isContract) {
isContract = _addr.code.length > 0;
}With the wrap() function, NFTs are transferred to the contract and in case of airdrop due to these NFTs, it will be stuck in the contract as there is no function to take these airdrop tokens from the contract.
Important NFT project owners are given airdrops, especially since the project includes NFTs such as BAYC, Moonbirds, Doodles, Azuki, there is a high probability of receiving Airdrops, but there is no function to withdraw incoming airdrop tokens, so airdrop tokens will be stuck in the contract.
A common method for airdrops is to collect airdrops with claim, so the Pair.sol contract can be considered upgradagable, adding a function to make claim.
src/Pair.sol:
216 /// @return fractionalTokenAmount The amount of fractional tokens minted.
217: function wrap(uint256[] calldata tokenIds, bytes32[][] calldata proofs)
218: public
219: returns (uint256 fractionalTokenAmount)
220: {
221: // *** Checks *** //
222:
223: // check that wrapping is not closed
224: require(closeTimestamp == 0, "Wrap: closed");
225:
226: // check the tokens exist in the merkle root
227: _validateTokenIds(tokenIds, proofs);
228:
229: // *** Effects *** //
230:
231: // mint fractional tokens to sender
232: fractionalTokenAmount = tokenIds.length * ONE;
233: _mint(msg.sender, fractionalTokenAmount);
234:
235: // *** Interactions *** //
236:
237: // transfer nfts from sender
238: for (uint256 i = 0; i < tokenIds.length; i++) {
239: ERC721(nft).safeTransferFrom(msg.sender, address(this), tokenIds[i]);
240: }
241:
242: emit Wrap(tokenIds);
243: }
Add this code:
/**
* @notice Sends ERC20 tokens trapped in contract to external address
* @dev Onlyowner is allowed to make this function call
* @param account is the receiving address
* @param externalToken is the token being sent
* @param amount is the quantity being sent
* @return boolean value indicating whether the operation succeeded.
*
*/
function rescueERC20(address account, address externalToken, uint256 amount) public onlyOwner returns (bool) {
IERC20(externalToken).transfer(account, amount);
return true;
}
}
Description:
The test coverage rate of the project is 97%. Testing all functions is best practice in terms of security criteria.
| File | % Lines | % Statements | % Branches | % Funcs |
|------------------------------------------|------------------|-------------------|----------------|----------------|
| src/Caviar.sol | 100.00% (11/11) | 100.00% (15/15) | 100.00% (4/4) | 100.00% (2/2) |
| src/LpToken.sol | 100.00% (2/2) | 100.00% (2/2) | 100.00% (0/0) | 100.00% (2/2) |
| src/Pair.sol | 100.00% (88/88) | 100.00% (107/107) | 95.24% (40/42) | 86.36% (19/22) |
| src/lib/SafeERC20Namer.sol | 0.00% (0/38) | 0.00% (0/53) | 0.00% (0/12) | 0.00% (0/7) |Due to its capacity, test coverage is expected to be 100%.
Context:
All Contracts
Description:
It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation.
In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.
https://docs.soliditylang.org/en/v0.8.15/natspec-format.html
Recommendation:
NatSpec comments should be increased in contracts
Context:
All Contracts
Description:
Order of Functions; ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.
https://docs.soliditylang.org/en/v0.8.17/style-guide.html
Functions should be grouped according to their visibility and ordered:
- constructor
- receive function (if exists)
- fallback function (if exists)
- external
- public
- internal
- private
- within a grouping, place the view and pure functions last
foundry.toml:
1: [profile.default]
2: src = "src"
3: out = "out"
4: libs = ["lib"]
5: solc = "0.8.17"
6: optimizer_runs = 3_000Description:
Protocol has enabled optional compiler optimizations in Solidity.
There have been several optimization bugs with security implications. Moreover, optimizations are actively being developed. 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. More recently, another bug due to the incorrect caching of keccak256 was reported.
A compiler audit of Solidity from November 2018 concluded that the optional optimizations may not be safe.
It is likely that there are latent bugs related to optimization and that new bugs will be introduced due to future optimizations.
Exploit Scenario:
A latent or future bug in Solidity compiler optimizations—or in the Emscripten transpilation to solc-js—causes a security vulnerability in the contracts.
Recommendation:
Short term, measure the gas savings from optimizations and carefully weigh them against the possibility of an optimization-related bug.
Long term, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.
Context:
13 results - 4 files
src/Caviar.sol:
3
4: import "solmate/auth/Owned.sol";
5
6: import "./lib/SafeERC20Namer.sol";
7: import "./Pair.sol";
8
src/LpToken.sol:
3
4: import "solmate/auth/Owned.sol";
5: import "solmate/tokens/ERC20.sol";
6
src/Pair.sol:
3
4: import "solmate/tokens/ERC20.sol";
5: import "solmate/tokens/ERC721.sol";
6: import "solmate/utils/MerkleProofLib.sol";
7: import "solmate/utils/SafeTransferLib.sol";
8: import "openzeppelin/utils/math/Math.sol";
9
10: import "./LpToken.sol";
11: import "./Caviar.sol";
12
src/lib/SafeERC20Namer.sol:
3
4: import "openzeppelin/utils/Strings.sol";
5
Description:
Solidity code is also cleaner in another way that might not be noticeable: the struct Point. We were importing it previously with global import but not using it. The Point struct polluted the source code with an unnecessary object we were not using because we did not need it.
This was breaking the rule of modularity and modular programming: only import what you need Specific imports with curly braces allow us to apply this rule better.
Recommendation:
import {contract1 , contract2} from "filename.sol";
A good example from the ArtGobblers project;
import {Owned} from "solmate/auth/Owned.sol";
import {ERC721} from "solmate/tokens/ERC721.sol";
import {LibString} from "solmate/utils/LibString.sol";
import {MerkleProofLib} from "solmate/utils/MerkleProofLib.sol";
import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";
import {ERC1155, ERC1155TokenReceiver} from "solmate/tokens/ERC1155.sol";
import {toWadUnsafe, toDaysWadUnsafe} from "solmate/utils/SignedWadMath.sol";Description:
Pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or EthPM package. Otherwise, the developer would need to manually update the pragma in order to compile locally.
https://swcregistry.io/docs/SWC-103
Recommendation:
Ethereum Smart Contract Best Practices - Lock pragmas to specific compiler version.
solidity-specific/locking-pragmas
5 results - 4 files
src/Caviar.sol:
1 // SPDX-License-Identifier: MIT
2: pragma solidity ^0.8.17;
3
src/LpToken.sol:
1 // SPDX-License-Identifier: MIT
2: pragma solidity ^0.8.17;
3
src/Pair.sol:
1 // SPDX-License-Identifier: MIT
2: pragma solidity ^0.8.17;
3
src/lib/SafeERC20Namer.sol:
1 // SPDX-License-Identifier: MIT
2: pragma solidity ^0.8.17;
2 results - 1 file
src/Pair.sol:
399: return (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997);
413: return (inputAmountWithFee * baseTokenReserves()) / ((fractionalTokenReserves() * 1000) + inputAmountWithFee);
Description:
There are occasions where certain numbers have been hardcoded, either in variable or in the code itself. Large numbers can become hard to read.
Recommendation:
Consider using underscores for number literals to improve its readability.
1 result - 1 file
src/Pair.sol:
473 for (uint256 i = 0; i < tokenIds.length; i++) {
474: bool isValid = MerkleProofLib.verify(proofs[i], merkleRoot, keccak256(abi.encodePacked(tokenIds[i])));
Rather than using abi.encodePacked for appending bytes, since version 0.8.4, bytes.concat() is enabled.
Since version 0.8.4 for appending bytes, bytes.concat() can be used instead of abi.encodePacked(,)
https://github.com/ethereum/solidity/blob/develop/Changelog.md
0.8.17 (2022-09-08)
0.8.16 (2022-08-08)
0.8.15 (2022-06-15)
0.8.10 (2021-11-09)
Unexpected bugs can be reported in recent versions;
Risks related to recent releases
Risks of complex code generation changes
Risks of new language features
Risks of known bugs
Use a non-legacy and more battle-tested version
Use 0.8.10
Consider adding EIP-2981 NFT Royalty Standard to the project
https://eips.ethereum.org/EIPS/eip-2981
Royalty (Copyright – EIP 2981):
- Fixed % royalties: For example, 6% of all sales go back to artists
- Declining royalties: There may be a continuous decline in sales based on time or any other variable.
- Dynamic royalties: Varies over time or sales amount
- Upgradeable royalties: Allows a legal entity or NFT owner to change any copyright
- Incremental royalties: No royalties, for example when sold for less than
$100 - Managed royalties: Funds are owned by a DAO, imagine the recipient is a DAO treasury
- Royalties to different people: Collectors and artists can even use royalties, not specific to a particular personality
[N-11] Showing the actual values of numbers in NatSpec comments makes checking and reading code easier
src/Pair.sol:
19
- 20: uint256 public constant ONE = 1e18
+ 20: uint256 public constant ONE = 1e18; // 1_000_000_000_000_000_000
- 21: uint256 public constant CLOSE_GRACE_PERIOD = 7 days;
+ 21: uint256 public constant CLOSE_GRACE_PERIOD = 7 days; // 604_800 ( 7 * 24 * 60 * 60)Context:
src/Pair.sol:
38
39: constructor(
40: address _nft,
41: address _baseToken,
42: bytes32 _merkleRoot,
43: string memory pairSymbol,
44: string memory nftName,
45: string memory nftSymbol
46: ) ERC20(string.concat(nftName, " fractional token"), string.concat("f", nftSymbol), 18) {
47: nft = _nft;
48: baseToken = _baseToken; // use address(0) for native ETH
49: merkleRoot = _merkleRoot;
50: lpToken = new LpToken(pairSymbol);
51: caviar = Caviar(msg.sender);
52: }
src/LpToken.sol:
11 contract LpToken is Owned, ERC20 {
12: constructor(string memory pairSymbol)
13: Owned(msg.sender)
14: ERC20(string.concat(pairSymbol, " LP token"), string.concat("LP-", pairSymbol), 18)
15: {}
src/Pair.sol:
38
39: constructor(
40: address _nft,
41: address _baseToken,
42: bytes32 _merkleRoot,
43: string memory pairSymbol,
44: string memory nftName,
45: string memory nftSymbol
46: ) ERC20(string.concat(nftName, " fractional token"), string.concat("f", nftSymbol), 18) {
47: nft = _nft;
48: baseToken = _baseToken; // use address(0) for native ETH
49: merkleRoot = _merkleRoot;
50: lpToken = new LpToken(pairSymbol);
51: caviar = Caviar(msg.sender);
52: }Description:
Events help non-contract tools to track changes, and events prevent users from being surprised by changes
Recommendation:
Add Event-Emit
NFT thefts have increased recently, so with the addition of hacked NFTs to the platform, NFTs can be converted into liquidity. To prevent this, I recommend adding the blacklist function.
Marketplaces such as Opensea have a blacklist feature that will not list NFTs that have been reported theft, NFT projects such as Manifold have blacklist functions in their smart contracts.
Here is the project example; Manifold
Manifold Contract
https://etherscan.io/address/0xe4e4003afe3765aca8149a82fc064c0b125b9e5a#code
modifier nonBlacklistRequired(address extension) {
require(!_blacklistedExtensions.contains(extension), "Extension blacklisted");
_;
}Recommended Mitigation Steps:
Add to Blacklist function and modifier.
At the start of the project, the system may need to be stopped or upgraded, I suggest you have a script beforehand and add it to the documentation.
This can also be called an " EMERGENCY STOP (CIRCUIT BREAKER) PATTERN ".
https://github.com/maxwoe/solidity_patterns/blob/master/security/EmergencyStop.sol
Description:
I recommend using header for Solidity code layout and readability
https://github.com/transmissions11/headers
/*//////////////////////////////////////////////////////////////
TESTING 123
//////////////////////////////////////////////////////////////*/Great report
Great report by the warden!
For this contest, 31 reports were submitted by wardens detailing gas optimizations. The report highlighted below by Rolezn received the top score from the judge.
The following wardens also submitted reports: SleepingBugs, JC, Breeje, Aymen0909, c3phas, IllIllI, lukris02, cryptonue, Madalad, Diana, ret2basic, tnevler, 0xAgro, carlitox477, gz627, RaymondFam, 0xSmartContract, NoamYakov, 0xab00, JrNet, HardlyCodeMan, pavankv, Bnke0x0, ReyAdmirado, oyc_109, millersplanet, saneryee, 0x1f8b, UdarTeam, and 0xhacksmithh.
| Issue | Contexts | Estimated Gas Saved | |
|---|---|---|---|
| G‑01 | <x> += <y> Costs More Gas Than <x> = <x> + <y> For State Variables |
3 | - |
| G‑02 | ++i/i++ Should Be unchecked{++i}/unchecked{i++} When It Is Not Possible For Them To Overflow, As Is The Case When Used In For- And While-loops |
7 | 245 |
| G‑03 | require()/revert() Strings Longer Than 32 Bytes Cost Extra Gas |
4 | - |
| G‑04 | Splitting require() Statements That Use && Saves Gas |
1 | 9 |
| G‑05 | Public Functions To External | 18 | - |
| G‑06 | Optimize names to save gas | 3 | 66 |
| G‑07 | Using fixed bytes is cheaper than using string |
2 | - |
| G‑08 | Superfluous event fields | 1 | - |
| G‑09 | internal functions only called once can be inlined to save gas |
3 | - |
| G‑10 | Setting the constructor to payable |
3 | 39 |
| G‑11 | Functions guaranteed to revert when called by normal users can be marked payable |
2 | 42 |
| G‑12 | Using unchecked blocks to save gas |
1 | 136 |
Total: 48 contexts over 12 issues
448: balanceOf[from] -= amount;
453: balanceOf[to] += amount;https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L448
https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L453
35: charCount += uint8(b[i]);https://github.com/code-423n4/2022-12-caviar/tree/main/src/lib\SafeERC20Namer.sol#L35
[G-02] ++i/i++ Should Be unchecked{++i}/unchecked{i++} When It Is Not Possible For Them To Overflow, As Is The Case When Used In For- And While-loops
The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas PER LOOP
238: for (uint256 i = 0; i < tokenIds.length; i++) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L238
258: for (uint256 i = 0; i < tokenIds.length; i++) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L258
468: for (uint256 i = 0; i < tokenIds.length; i++) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L468
13: for (uint256 j = 0; j < 32; j++) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/lib\SafeERC20Namer.sol#L13
22: for (uint256 j = 0; j < charCount; j++) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/lib\SafeERC20Namer.sol#L22
33: for (uint256 i = 32; i < 64; i++) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/lib\SafeERC20Namer.sol#L33
39: for (uint256 i = 0; i < charCount; i++) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/lib\SafeERC20Namer.sol#L39
51: require(msg.sender == pairs[nft][baseToken][merkleRoot], "Only pair can destroy itself");https://github.com/code-423n4/2022-12-caviar/tree/main/src/Caviar.sol#L51
80: require(lpTokenAmount >= minLpTokenAmount, "Slippage: lp token amount out");https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L80
117: require(baseTokenOutputAmount >= minBaseTokenOutputAmount, "Slippage: base token amount out");https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L117
120: require(fractionalTokenOutputAmount >= minFractionalTokenOutputAmount, "Slippage: fractional token out");https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L120
Instead of using operator && on a single require. Using a two require can save more gas.
i.e.
for require(version == 1 && _bytecodeHash[1] == bytes1(0), "zf"); use:
require(version == 1);
require(_bytecodeHash[1] == bytes1(0));
71: require(baseTokenAmount > 0 && fractionalTokenAmount > 0, "Input token amount is zero");https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L71
The following functions could be set external to save gas and improve code quality.
External call cost is less expensive than of public functions.
function create(address nft, address baseToken, bytes32 merkleRoot) public returns (Pair pair) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/Caviar.sol#L28
function destroy(address nft, address baseToken, bytes32 merkleRoot) public {https://github.com/code-423n4/2022-12-caviar/tree/main/src/Caviar.sol#L49
function mint(address to, uint256 amount) public onlyOwner {https://github.com/code-423n4/2022-12-caviar/tree/main/src/LpToken.sol#L19
function burn(address from, uint256 amount) public onlyOwner {https://github.com/code-423n4/2022-12-caviar/tree/main/src/LpToken.sol#L26
function buy(uint256 outputAmount, uint256 maxInputAmount) public payable returns (uint256 inputAmount) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L147
function sell(uint256 inputAmount, uint256 minOutputAmount) public returns (uint256 outputAmount) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L182
function unwrap(uint256[] calldata tokenIds) public returns (uint256 fractionalTokenAmount) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L248
function nftAdd(
uint256 baseTokenAmount,
uint256[] calldata tokenIds,
uint256 minLpTokenAmount,
bytes32[][] calldata proofs
) public payable returns (uint256 lpTokenAmount) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L275
function nftBuy(uint256[] calldata tokenIds, uint256 maxInputAmount) public payable returns (uint256 inputAmount) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L310
function close() public {https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L341
function withdraw(uint256 tokenId) public {https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L359
function baseTokenReserves() public view returns (uint256) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L379
function fractionalTokenReserves() public view returns (uint256) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L383
function price() public view returns (uint256) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L390
function buyQuote(uint256 outputAmount) public view returns (uint256) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L398
function sellQuote(uint256 inputAmount) public view returns (uint256) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L406
function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L417
function removeQuote(uint256 lpTokenAmount) public view returns (uint256, uint256) {https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L435
Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions.
See more here.
File: .\Projects\caviar202212\2022-12-caviar\src\Caviar.solhttps://github.com/code-423n4/2022-12-caviar/tree/main/src/Caviar.sol
File: .\Projects\caviar202212\2022-12-caviar\src\LpToken.solhttps://github.com/code-423n4/2022-12-caviar/tree/main/src/LpToken.sol
File: .\Projects\caviar202212\2022-12-caviar\src\Pair.solhttps://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol
Find a lower method ID name for the most called functions for example Call() vs. Call1() is cheaper by 22 gas.
For example, the function IDs in the Gauge.sol contract will be the most used; A lower method ID may be given.
As a rule of thumb, use bytes for arbitrary-length raw byte data and string for arbitrary-length string (UTF-8) data. If you can limit the length to a certain number of bytes, always use one of bytes1 to bytes32 because they are much cheaper.
33: string memory baseTokenSymbol = baseToken == address(0) ? "ETH" : baseToken.tokenSymbol();https://github.com/code-423n4/2022-12-caviar/tree/main/src/Caviar.sol#L33
36: string memory pairSymbol = string.concat(nftSymbol, ":", baseTokenSymbol);https://github.com/code-423n4/2022-12-caviar/tree/main/src/Caviar.sol#L36
block.number and block.timestamp are added to the event information by default, so adding them manually will waste additional gas.
36: event Close(uint256 closeTimestamp);https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L36
463: function _validateTokenIdshttps://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L463
76: function tokenSymbolhttps://github.com/code-423n4/2022-12-caviar/tree/main/src/lib\SafeERC20Namer.sol#L76
87: function tokenNamehttps://github.com/code-423n4/2022-12-caviar/tree/main/src/lib\SafeERC20Namer.sol#L87
Saves ~13 gas per instance
21: constructor() Owned(msg.sender)https://github.com/code-423n4/2022-12-caviar/tree/main/src/Caviar.sol#L21
11: constructor(string memory pairSymbol)
Owned(msg.sender)
ERC20(string.concat(pairSymbol, " LP token"), string.concat("LP-", pairSymbol), 18)https://github.com/code-423n4/2022-12-caviar/tree/main/src/LpToken.sol#L11
39: constructor(
address _nft,
address _baseToken,
bytes32 _merkleRoot,
string memory pairSymbol,
string memory nftName,
string memory nftSymbol
) ERC20(string.concat(nftName, " fractional token"), string.concat("f", nftSymbol), 18)https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L39
If a function modifier or require such as onlyOwner/onlyX is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2) which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.
19: function mint(address to, uint256 amount) public onlyOwner {https://github.com/code-423n4/2022-12-caviar/tree/main/src/LpToken.sol#L19
26: function burn(address from, uint256 amount) public onlyOwner {https://github.com/code-423n4/2022-12-caviar/tree/main/src/LpToken.sol#L26
Functions guaranteed to revert when called by normal users can be marked payable.
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.
168: uint256 refundAmount = maxInputAmount - inputAmount;https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L168
C4 is an open organization governed by participants in the community.
C4 Contests incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Contest 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.