Platform: Code4rena
Start Date: 20/01/2022
Pot Size: $50,000 USDC
Total HM: 3
Participants: 35
Period: 7 days
Judge: GalloDaSballo
Total Solo HM: 2
Id: 77
League: ETH
Rank: 1/35
Findings: 5
Award: $33,760.19
🌟 Selected for report: 10
🚀 Solo Findings: 2
🌟 Selected for report: WatchPug
15454.9951 USDC - $15,455.00
WatchPug
baseToken rebase upPer the document: https://github.com/ElasticSwap/elasticswap/blob/a90bb67e2817d892b517da6c1ba6fae5303e9867/ElasticSwapMath.md#:~:text=When%20there%20is%20alphaDecay
and related code: https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/libraries/MathLib.sol#L227-L283
Gamma is the ratio of shares received by the new liquidity provider when addLiquidity() (ΔRo) to the new totalSupply (total shares = Ro' = Ro + ΔRo).
ΔRo = (Ro/(1 - γ)) * γ Ro * Gamma = -------------- 1 - Gamma ⟺ ΔRo * ( 1 - Gamma ) = Gamma * Ro ΔRo - Gamma * ΔRo = Gamma * Ro ΔRo = Gamma * Ro + Gamma * ΔRo ΔRo Gamma = --------- Ro + ΔRo
In the current implementation:
γ = ΔY / Y' / 2 * ( ΔX / α^ )
ΔY is the quoteToken added by the new liquidity provider. See:
Y' is the new Y after addLiquidity(), Y' = Y + ΔY. See:
ΔX is ΔY * Omega. See:
α^ is Alpha - X. See:
For instance:
Given:
baseToken rebase up: Alpha becomes 10When: new liquidity provider addLiquidity() with 4 quoteToken:
4 4 * Omega 16 Gamma = ------------ * ------------ = ---- (1+4) * 2 10 - 1 90
After addLiquidity():
quoteToken, the total value is: 160 / 90 / Omega + 80 / 90 = 240 / 90 = 2.6666666666666665As a result, the new liquidity provider suffers a fund loss of 4 - 240 / 90 = 1.3333333333333333 in the terms of quoteToken
The case above can be reproduced by changing the numbers in this test unit.
baseToken rebase downPer the document: https://github.com/ElasticSwap/elasticswap/blob/a90bb67e2817d892b517da6c1ba6fae5303e9867/ElasticSwapMath.md#:~:text=When%20there%20is%20betaDecay
and related code: https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/libraries/MathLib.sol#L297-L363
Gamma is the ratio of shares received by the new liquidity provider when addLiquidity() (ΔRo) to the new totalSupply (total shares = Ro' = Ro + ΔRo).
ΔRo = (Ro/(1 - γ)) * γ Ro * Gamma = -------------- 1 - Gamma ⟺ ΔRo * ( 1 - Gamma ) = Gamma * Ro ΔRo - Gamma * ΔRo = Gamma * Ro ΔRo = Gamma * Ro + Gamma * ΔRo ΔRo Gamma = --------- Ro + ΔRo
In the current implementation:
γ = ΔX / X / 2 * ( ΔXByQuoteTokenAmount / β^ )
ΔX is the amount of baseToken added by the new liquidity provider. See:
X is the balanceOf baseToken. See:
ΔXByQuoteTokenAmount is ΔX / Omega, the value of ΔX in the terms of quoteToken. See:
β^ is maxΔX / Omega, the value of maxΔX in the terms of quoteToken. maxΔX = X - Alpha. See:
For instance:
Given:
baseToken rebase down, Alpha becomes 1When: new liquidity provider addLiquidity() with 4 baseToken
4 4 / Omega 8 Gamma = -------- * ---------------- = ---- 10 * 2 (10-1) / Omega 90
After addLiquidity():
As a result, the new liquidity provider suffers a fund loss of 4 - 120 / 90 = 2.6666666666666665 in the terms of quoteToken
The case above can be reproduced by changing the numbers in this test unit.
When: new liquidity provider addLiquidity with ΔY quoteToken (ΔY <= maxΔY or ΔY <= α^ / ω) After addLiquidity(): - baseToken belongs to the newLP: ΔXOfNewLP - quoteToken belongs to the newLP: ΔYOfNewLP ΔY can be divided into 2 parts: - ΔYToX: the part used for swap ΔXOfNewLP. ΔXOfNewLP = ΔYToX * Omega (a1) - ΔY - ΔYToX: the rest as ΔYOfNewLP The ratio of newly minted LP tokens for new liquidity provider to the new totalSupply (Ro'): Gamma ΔRo ΔY - ΔYToX ΔXOfNewLP = --------- = ----------- = ----------- (a2) Ro + ΔRo Y + ΔY Alpha ΔY - ΔYToX ΔYToX * Omega = ----------- = --------------- // substituting (a1) (a_exp1) Y + ΔY Alpha ⟺ (ΔY - ΔYToX) * Alpha = ΔYToX * Omega * (Y + ΔY) ΔY * Alpha - ΔYToX * Alpha = ΔYToX * Omega * (Y + ΔY) ΔY * Alpha = ΔYToX * Alpha + ΔYToX * Omega * (Y + ΔY) = ΔYToX * ( Alpha + Omega * (Y + ΔY)) ΔY * Alpha ΔY * Alpha ΔYToX = --------------------------- = -------------------- (a_r1) Alpha + Omega * (Y + ΔY) Alpha + Omega * Y' Continue from (a_exp1): ΔYToX * Omega Gamma = --------------- Alpha ΔY * Omega = ----------------------- // substituting (a_r1) (a_r2(1)) Alpha + Omega * Y' ΔY = --------------------- (a_r2(2)) Alpha/Omega + Y' Gamma is the ratio of ΔY to the total amounts of baseToken and quoteToken after addLiquidity: - (a_r2(1)) is the formula in the terms of baseToken - (a_r2(2)) is the formula in the terms of quoteToken Based on (a2): ΔRo Gamma = --------- Ro + ΔRo ⟺ ΔRo = Gamma * Ro + Gamma * ΔRo ΔRo - Gamma * ΔRo = Gamma * Ro ΔRo * ( 1 - Gamma ) = Gamma * Ro Ro * Gamma ΔRo = -------------- 1 - Gamma
When: new liquidity provider addLiquidity with ΔX baseToken (ΔX <= maxΔX or ΔY <= α^) After addLiquidity() - baseToken belongs to the newLP: ΔXOfNewLP - quoteToken belongs to the newLP: ΔYOfNewLP ΔX can be divided into 2 parts: - ΔXToY: the part used for swap ΔYOfNewLP. ΔYOfNewLP = ΔXToY / Omega (b1) - ΔX - ΔXToY: the rest as ΔXOfNewLP The ratio of newly minted LP tokens for new liquidity provider to the new totalSupply (Ro'): Gamma ΔRo ΔX - ΔXToY ΔYOfNewLP = --------- = ------------ = ------------ (b2) Ro + ΔRo Alpha + ΔX Y ΔXToY / Omega = ------------- // substituting (b1) Y ΔXToY = ------------- (b_exp1) Y * Omega ⟺ (ΔX - ΔXToY) * Y = (Alpha + ΔX) * ΔXToY / Omega ΔX * Y - ΔXToY * Y = (Alpha + ΔX) * ΔXToY / Omega ΔX * Y = ΔXToY * Y + (Alpha + ΔX) * ΔXToY / Omega = ΔXToY * ( Y + (Alpha + ΔX) / Omega ) ΔX * Y ΔXToY = -------------------------- (b_r1) Y + (Alpha + ΔX) / Omega Continue from (b_exp1) ΔXToY Gamma = ------------- Y * Omega ΔX * Y = ---------------------------------------- // substituting (b_r1) (Y + (Alpha + ΔX) / Omega) * Y * Omega ΔX / Omega = --------------------------- (b_r2(2)) Y + (Alpha + ΔX) / Omega ΔX = --------------------------- (b_r2(1)) Y * Omega + (Alpha + ΔX) ΔX = ------------------- // substituting (Omega = X/Y) X + (Alpha + ΔX) Gamma is the ratio of ΔX to the total amounts of baseToken and quoteToken after addLiquidity: - (b_r2(1)) is the formula in the terms of baseToken - (b_r2(2)) is the formula in the terms of quoteToken Based on (b2): ΔRo Gamma = --------- Ro + ΔRo ⟺ Ro * Gamma ΔRo = -------------- 1 - Gamma
Update code and document using the correct formula for ΔRo.
#0 - 0xean
2022-01-27T13:44:19Z
@0xSSDD - please review.
#1 - GalloDaSballo
2022-02-05T13:44:12Z
@0xSSDD @0xean Can I get confirmation on your review?
#2 - 0xean
2022-02-05T14:20:01Z
Finding is valid - solution seems to be partially correct and we are working on the fully correct version.
It seems that the suggested formula doesn’t cover a rebase down correctly and this is where our efforts are focused now.
On Feb 5, 2022, at 6:44 AM, Alex The Entreprenerd @.***> wrote:
 @0xSSDD @0xean Can I get confirmation on your review?
— Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.
#3 - GalloDaSballo
2022-02-13T17:52:26Z
The warden has identified an issue with the math that reliably will provide a less-than-expected value to single-sided liquidity providers. The warden showed a consistent way for this to occur and while the recommended fix may not be completely correct, I believe the finding to be valid.
Because the warden found a set of cases that reliably make the protocol return less value than expected when compared to the goals of the protocol, I believe High Severity to be appropriate
#4 - 0xean
2022-02-16T02:57:47Z
closed mistakenly due to linked PR - will leave open for judging report.
🌟 Selected for report: WatchPug
15454.9951 USDC - $15,455.00
WatchPug
In the current implementation, the amount of LP tokens to be minted when addLiquidity() is calculated based on the ratio between the amount of newly added quoteToken and the current wallet balance of quoteToken in the Exchange contract.
However, since anyone can transfer quoteToken to the contract, and make the balance of quoteToken to be larger than _internalBalances.quoteTokenReserveQty, existing liquidity providers can take advantage of this by donating quoteToken and make future liquidity providers receive fewer LP tokens than expected and lose funds.
liquidityTokenQty = calculateLiquidityTokenQtyForDoubleAssetEntry( _totalSupplyOfLiquidityTokens, quoteTokenQty, _quoteTokenReserveQty // IERC20(quoteToken).balanceOf(address(this)) );
Given:
Exchange pool is new;addLiquidity() with 1e18 baseToken and 1e18 quoteToken, recived 1e18 LP token;99e18 quoteToken to the Exchange pool contract;addLiquidity() with 1e18 baseToken and 1e18 quoteToken;removeLiquidity() with all the LP token in balance.Expected Results: Bob recived 1e18 baseToken and >= 1e18 quoteToken.
Actual Results: Bob recived ~0.02e18 baseToken and ~1e18 quoteToken.
Alice can now removeLiquidity() and recive ~1.98e18 baseToken and ~100e18 quoteToken.
As a result, Bob suffers a fund loss of 0.98e18 baseToken.
Change to:
liquidityTokenQty = calculateLiquidityTokenQtyForDoubleAssetEntry( _totalSupplyOfLiquidityTokens, quoteTokenQty, _internalBalances.quoteTokenReserveQty );
#0 - 0xean
2022-01-27T13:35:35Z
This does appear to be correct after attempting a POC. Thank you WatchPug!
For my own reference later:
it.only("POC", async () => { // create expiration 50 minutes from now. const expiration = Math.round(new Date().getTime() / 1000 + 60 * 50); const liquidityProvider = accounts[1]; const liquidityProvider2 = accounts[2]; // send users (liquidity provider) base and quote tokens for easy accounting. const liquidityProviderInitialBalances = 100000000; await baseToken.transfer( liquidityProvider.address, liquidityProviderInitialBalances ); await quoteToken.transfer( liquidityProvider.address, liquidityProviderInitialBalances ); await quoteToken.transfer( liquidityProvider2.address, liquidityProviderInitialBalances ); await baseToken.transfer( liquidityProvider2.address, liquidityProviderInitialBalances ); // add approvals await quoteToken .connect(liquidityProvider) .approve(exchange.address, liquidityProviderInitialBalances); await baseToken .connect(liquidityProvider) .approve(exchange.address, liquidityProviderInitialBalances); await quoteToken .connect(liquidityProvider2) .approve(exchange.address, liquidityProviderInitialBalances); await baseToken .connect(liquidityProvider2) .approve(exchange.address, liquidityProviderInitialBalances); const baseTokenLiquidityToAdd = 50000; const quoteTokenLiquidityToAdd = 50000; await exchange.connect(liquidityProvider).addLiquidity( baseTokenLiquidityToAdd, // base token quoteTokenLiquidityToAdd, // quote token 1, 1, liquidityProvider.address, expiration ); await quoteToken .connect(liquidityProvider) .transfer(exchange.address, quoteTokenLiquidityToAdd); await exchange .connect(liquidityProvider2) .addLiquidity( baseTokenLiquidityToAdd, quoteTokenLiquidityToAdd, 1, 1, liquidityProvider2.address, expiration ); await exchange .connect(liquidityProvider) .removeLiquidity( await exchange.balanceOf(liquidityProvider.address), 1, 1, liquidityProvider.address, expiration ); await exchange .connect(liquidityProvider2) .removeLiquidity( await exchange.balanceOf(liquidityProvider2.address), 1, 1, liquidityProvider2.address, expiration ); }); });
#1 - GalloDaSballo
2022-02-13T17:39:16Z
The warden identified a way to exploit the protocol math to devalue future liquidity provision at the advantage of early liquidity providers.
The exploit is extractive in nature, however, because this is reliably performable and effectively breaks the protocol's goals and mechanics, I believe High Severity to be appropriate
845.0019 USDC - $845.00
WatchPug
For the first minter of an Exchange pool, the ratio of X/Y and the totalSupply of the LP token can be manipulated.
A sophisticated attacker can mint and burn all of the LP tokens but 1 Wei, and then artificially create a situation of rebasing up by transferring baseToken to the pool contract. Then addLiquidity() in singleAssetEntry mode.
Due to the special design of singleAssetEntry mode, the value of LP token can be inflated very quickly.
As a result, 1 Wei of LP token can be worthing a significate amount of baseToken and quoteToken.
Combine this with the precision loss when calculating the amount of LP tokens to be minted to the new liquidity provider, the attacker can turn the pool into a trap which will take a certain amount of cut for all future liquidity providers by minting fewer LP tokens to them.
} else { // this user will set the initial pricing curve require( _baseTokenQtyDesired > 0, "MathLib: INSUFFICIENT_BASE_QTY_DESIRED" ); require( _quoteTokenQtyDesired > 0, "MathLib: INSUFFICIENT_QUOTE_QTY_DESIRED" ); tokenQtys.baseTokenQty = _baseTokenQtyDesired; tokenQtys.quoteTokenQty = _quoteTokenQtyDesired; tokenQtys.liquidityTokenQty = sqrt( _baseTokenQtyDesired * _quoteTokenQtyDesired ); _internalBalances.baseTokenReserveQty += tokenQtys.baseTokenQty; _internalBalances.quoteTokenReserveQty += tokenQtys.quoteTokenQty; }
function calculateLiquidityTokenQtyForDoubleAssetEntry( uint256 _totalSupplyOfLiquidityTokens, uint256 _quoteTokenQty, uint256 _quoteTokenReserveBalance ) public pure returns (uint256 liquidityTokenQty) { liquidityTokenQty = (_quoteTokenQty * _totalSupplyOfLiquidityTokens) / _quoteTokenReserveBalance; }
Given:
Pool is newly created;baseToken in terms of quoteToken is 1.The attacker can do the following steps in one tx:
addLiquidity() with 2 Wei of baseToken and 100e18 quoteToken, received 14142135623 LP tokens;removeLiquidity() with 14142135622 LP tokens, the Pool state becomes:baseToken.transfer() 7071067812 Wei to the Pool contract;addLiquidity() with no baseToken and 50e18 quoteToken;swapBaseTokenForQuoteToken() with 600000000000000 baseToken, the Pool state becomes:baseToken.transfer() 999399992928932200 Wei to the Pool contract;addLiquidity() with no baseToken and 1e18 quoteToken, the Pool state becomes:From now on, addLiquidity() with less than 1e18 of baseToken and quoteToken will receive 0 LP token due to precision loss.
The amounts can be manipulated to higher numbers and cause most future liquidity providers to receive fewer LP tokens than expected, and the attacker will be able to profit from it as the attacker will take a larger share of the pool than expected.
Consider requiring a certain amount of minimal LP token amount (eg, 1e8) for the first minter and lock some of the first minter's LP tokens by minting ~1% of the initial amount to the factory address.
#0 - 0xean
2022-01-27T13:41:22Z
Thanks for the report, I don't agree with the severity based on this
From now on, addLiquidity() with less than 1e18 of baseToken and quoteToken will receive 0 LP token due to precision loss.
which in your example represents a user trying to add dust to the contract after the attack.
I think we will implement the minimum locked liquidity to avoid rounding errors, but this attack assumes users are adding dust to the contract and that they are totally unaware of the contract state which is incorrect. Users specific a min and a max token qty's when adding liquidity.
Would recommend med. risk on this one if not low risk given the attack is on "dust" amounts of tokens.
#1 - GalloDaSballo
2022-02-07T00:58:50Z
I agree with the finding and remember coming across it when reading the Yearn V0.4.2 Audit by Trails of Bits
Ultimately this is contingent on a donation, that will make each share more valuable, so it's effectively a way to use rounding against making dust donations.
Technically this same idea can be extended to huge donations, however there are very dubious economic reasons as to why you'd do that (perhaps frontrunning a moderate deposit with the goal of using this method to earn that MEV)
Ultimately this is something that can happen anytime you have X shares and Y totalSupply If the total supply reaches greater units than the shares, then integer division will inevitably eat out some of those shares.
Have yet to see a long term solution to this rounding problem, however, a simple initial addition that mints 1e18 shares will require some economic commitment by the potential exploiters
Agree with medium severity
🌟 Selected for report: WatchPug
1545.4995 USDC - $1,545.50
WatchPug
require( _quoteTokenQtyMin < maxQuoteTokenQty, "MathLib: INSUFFICIENT_DECAY" );
Should be changed to:
require( _quoteTokenQtyMin <= quoteTokenQty, "MathLib: INSUFFICIENT_DECAY" );
require the actual quoteTokenQty >= _quoteTokenQtyMin, because the actual quoteTokenQty can be < maxQuoteTokenQty.
see /elasticswap/src/libraries/MathLib.sol#L256< should be <=, to allow _quoteTokenQtyMin == _quoteTokenQtyDesiredSimilarly:
require( _baseTokenQtyMin < maxBaseTokenQty, "MathLib: INSUFFICIENT_DECAY" );
Should be changed to:
require( _baseTokenQtyMin <= baseTokenQty, "MathLib: INSUFFICIENT_DECAY" );
#0 - 0xean
2022-01-27T13:03:31Z
Agree on the < vs <= but this check is actually checking decay as the revert message states and is correct as implemented.
#1 - GalloDaSballo
2022-02-06T15:44:52Z
@0xean It seems like the code wouldn't break if you used _quoteTokenQtyMin <= maxQuoteTokenQty
Is there a specific reason for wanting to use Strictly Less Than?
#2 - GalloDaSballo
2022-02-08T14:07:21Z
Have to hear back from the judge, but from my review it seems like the system wouldn't break if they changed the check from < to <=
Will hold on on this but I think the finding is valid at this time
#3 - GalloDaSballo
2022-02-13T17:31:20Z
I believe that while the code shouldn't break by implementing the warden suggestion, the spec detailed here: https://github.com/ElasticSwap/elasticswap/blob/develop/ElasticSwapMath.md
Requires some decay.
Will wait for sponsor confirmation but believe the code is fine as is
#4 - 0xean
2022-02-13T19:00:30Z
Sorry for the delay, was OOO.
I believe the functionality is correct here once we make the change from < to <=
We want to confirm that the minimum amount supplied the user is less than the max.
The desired amount will always be more than the minimum amount, I am not clear on the warden suggesting otherwise while referencing this line(https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/libraries/MathLib.sol#L256)
#5 - GalloDaSballo
2022-02-14T01:39:36Z
Thank you for the reply @0xean I don't think the suggestion in line 256 makes a difference because in the case of the value being equal setting either is fine
As for the "main aspect" of the finding, which is suggesting the usage of <= instead of <, as I mentioned before I don't believe it's a breaking change, so if you agree I'll mark the finding as valid
🌟 Selected for report: sorrynotsorry
Also found by: 0v3rf10w, Dravee, Meta0xNull, WatchPug, byterocket, defsec, robee, sirhashalot, ye0lde
3.377 USDC - $3.38
WatchPug
Every reason string takes at least 32 bytes.
Use short reason strings that fits in 32 bytes or it will become more expensive.
Instances include:
require( IERC20(baseToken).balanceOf(address(this)) == tokenQtys.baseTokenQty, "Exchange: FEE_ON_TRANSFER_NOT_SUPPORTED" );
require( _baseTokenQtyMin > 0 && _quoteTokenQtyMin > 0, "Exchange: MINS_MUST_BE_GREATER_THAN_ZERO" );
require( _baseTokenQtyDesired > 0, "MathLib: INSUFFICIENT_BASE_QTY_DESIRED" ); require( _quoteTokenQtyDesired > 0, "MathLib: INSUFFICIENT_QUOTE_QTY_DESIRED" );
require( baseTokenQtyDecayChange > 0, "MathLib: INSUFFICIENT_CHANGE_IN_DECAY" );
require(_baseToken != _quoteToken, "ExchangeFactory: IDENTICAL_TOKENS"); require( _baseToken != address(0) && _quoteToken != address(0), "ExchangeFactory: INVALID_TOKEN_ADDRESS" ); require( exchangeAddressByTokenAddress[_baseToken][_quoteToken] == address(0), "ExchangeFactory: DUPLICATE_EXCHANGE" );
require( _feeAddress != address(0) && _feeAddress != feeAddress_, "ExchangeFactory: INVAlID_FEE_ADDRESS" );
#0 - 0xean
2022-01-31T14:11:51Z
dupe of #159
19.4163 USDC - $19.42
WatchPug
Redundant code increase contract size and gas usage at deployment.
function calculateAddQuoteTokenLiquidityQuantities( uint256 _quoteTokenQtyDesired, uint256 _quoteTokenQtyMin, uint256 _baseTokenReserveQty, uint256 _totalSupplyOfLiquidityTokens, InternalBalances storage _internalBalances ) public returns (uint256 quoteTokenQty, uint256 liquidityTokenQty) {
return (quoteTokenQty, liquidityTokenQty);
L282, return (quoteTokenQty, liquidityTokenQty) is redundant.
#0 - GalloDaSballo
2022-02-04T00:33:08Z
I agree with the finding, because the variables are defined in the returns of the signature, there's no need to explicitly return them (although it helps with clarity)
#1 - 0xean
2022-02-16T02:32:57Z
5.0956 USDC - $5.10
WatchPug
For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.
For example:
if (rootK > rootKLast) { uint256 numerator = _totalSupplyOfLiquidityTokens * (rootK - rootKLast); uint256 denominator = (rootK * 5) + rootKLast; liquidityTokenFeeQty = numerator / denominator; }
rootK - rootKLast will never underflow.
if (a >= b) { return a - b; } return b - a;
a - b / b - a will never underflow.
#0 - 0xean
2022-01-31T13:54:45Z
dupe of #177
🌟 Selected for report: wuwe1
Also found by: Ruhum, WatchPug, csanuragjain, gzeon
13.9797 USDC - $13.98
WatchPug
In Exchange.sol#swapBaseTokenForQuoteToken() and Exchange.sol#swapQuoteTokenForBaseToken(), _baseTokenQty and _minQuoteTokenQty are required to be > 0.
In MathLib.calculateBaseTokenQty() and MathLib.calculateQuoteTokenQty(), the same input parameters are checked again, which is redundant.
function swapBaseTokenForQuoteToken( uint256 _baseTokenQty, uint256 _minQuoteTokenQty, uint256 _expirationTimestamp ) external nonReentrant() { isNotExpired(_expirationTimestamp); require( _baseTokenQty > 0 && _minQuoteTokenQty > 0, "Exchange: INSUFFICIENT_TOKEN_QTY" ); uint256 quoteTokenQty = MathLib.calculateQuoteTokenQty( _baseTokenQty, _minQuoteTokenQty, TOTAL_LIQUIDITY_FEE, internalBalances );
function calculateQuoteTokenQty( uint256 _baseTokenQty, uint256 _quoteTokenQtyMin, uint256 _liquidityFeeInBasisPoints, InternalBalances storage _internalBalances ) public returns (uint256 quoteTokenQty) { require( _baseTokenQty > 0 && _quoteTokenQtyMin > 0, "MathLib: INSUFFICIENT_TOKEN_QTY" );
#0 - GalloDaSballo
2022-02-04T22:59:53Z
Duplicate of #173
47.9414 USDC - $47.94
WatchPug
MathLib.InternalBalances public internalBalances = MathLib.InternalBalances(0, 0, 0);
Initialize internalBalances to the default state is redundant.
Change to MathLib.InternalBalances public internalBalances; can make the code simpler and save some gas.
#0 - GalloDaSballo
2022-02-04T00:38:29Z
Agree with the finding, for the sake of visibility the sponsor may decide to keep the assignment as a comment, but this is effectually paying for assigning the same value to a storage slot (should be 3 due to int types) which should cost 800 per assignment (2.4k gas in total)
47.9414 USDC - $47.94
WatchPug
The check of y > 3 is unnecessary and most certainly adds more gas cost than it saves as the majority of use cases of this function will not be handling y <= 3.
function sqrt(uint256 y) internal pure returns (uint256 z) { if (y > 3) { z = y; uint256 x = y / 2 + 1; while (x < z) { z = x; x = (y / x + x) / 2; } } else if (y != 0) { z = 1; } }
Change to:
function sqrt(uint x) public pure returns (uint y) { uint z = (x + 1) / 2; y = x; while (z < y) { y = z; z = (x / z + z) / 2; } }
Or use:
#0 - GalloDaSballo
2022-02-04T01:04:05Z
Agree with the finding
2.3145 USDC - $2.31
WatchPug
It is cheaper to use != 0 than > 0 for uint256.
if (_totalSupplyOfLiquidityTokens > 0) {
_baseTokenQtyDesired > 0,
_quoteTokenQtyDesired > 0,
require(_tokenAQty > 0, "MathLib: INSUFFICIENT_QTY"); require( _tokenAReserveQty > 0 && _tokenBReserveQty > 0, "MathLib: INSUFFICIENT_LIQUIDITY" );
#0 - GalloDaSballo
2022-02-18T18:29:52Z
Duplicate of #161
🌟 Selected for report: WatchPug
106.5364 USDC - $106.54
WatchPug
The _quoteTokenQtyMin parameter of calculateAddQuoteTokenLiquidityQuantities() is redundant.
calculateAddQuoteTokenLiquidityQuantities() is only used once, and the _quoteTokenQtyMin is always 0.
( quoteTokenQtyFromDecay, liquidityTokenQtyFromDecay ) = calculateAddQuoteTokenLiquidityQuantities( _quoteTokenQtyDesired, 0, // there is no minimum for this particular call since we may use quote tokens later. _baseTokenReserveQty, _totalSupplyOfLiquidityTokens, _internalBalances );
Removing _quoteTokenQtyMin and related code can save gas.
#0 - GalloDaSballo
2022-02-04T22:53:38Z
I believe this will save the cost of the calldata parameters as well as their handling in the bytecode
🌟 Selected for report: WatchPug
106.5364 USDC - $106.54
WatchPug
For the storage variables that will be accessed multiple times, cache and read from the stack can save ~100 gas from each extra read (SLOAD after Berlin).
For example:
uint256 baseTokenQtyToRemoveFromInternalAccounting = (_liquidityTokenQty * internalBalances.baseTokenReserveQty) / totalSupplyOfLiquidityTokens; internalBalances .baseTokenReserveQty -= baseTokenQtyToRemoveFromInternalAccounting; // We should ensure no possible overflow here. if (quoteTokenQtyToReturn > internalBalances.quoteTokenReserveQty) { internalBalances.quoteTokenReserveQty = 0; } else { internalBalances.quoteTokenReserveQty -= quoteTokenQtyToReturn; } internalBalances.kLast = internalBalances.baseTokenReserveQty * internalBalances.quoteTokenReserveQty;
internalBalances.baseTokenReserveQty and internalBalances.quoteTokenReserveQty can be cached.
Change to:
uint256 internalBaseTokenReserveQty = internalBalances.baseTokenReserveQty; uint256 baseTokenQtyToRemoveFromInternalAccounting = (_liquidityTokenQty * internalBaseTokenReserveQty) / totalSupplyOfLiquidityTokens; internalBalances .baseTokenReserveQty = internalBaseTokenReserveQty = internalBaseTokenReserveQty - baseTokenQtyToRemoveFromInternalAccounting; // We should ensure no possible overflow here. uint256 internalQuoteTokenReserveQty = internalBalances.quoteTokenReserveQty; if (quoteTokenQtyToReturn > internalQuoteTokenReserveQty) { internalBalances.quoteTokenReserveQty = internalQuoteTokenReserveQty = 0; } else { internalBalances.quoteTokenReserveQty = internalQuoteTokenReserveQty = internalQuoteTokenReserveQty - quoteTokenQtyToReturn; } internalBalances.kLast = internalBaseTokenReserveQty * internalQuoteTokenReserveQty;
#0 - GalloDaSballo
2022-02-04T22:55:00Z
I agree with the finding, for the second read the savings are 94 gas, (3 for writing and 3 for reading), for each subsequent reads the savings are 97 gas
🌟 Selected for report: WatchPug
106.5364 USDC - $106.54
WatchPug
Outdated versions of OpenZeppelin library are used.
New versions of OpenZeppelin libraries can be more gas efficient.
For example:
ERC20.sol in @openzeppelin/contracts@4.1.0:
require(currentAllowance >= amount, "ERC20: transfer amount exceeds allowance"); _approve(sender, _msgSender(), currentAllowance - amount);
A gas optimization upgrade has been added to @openzeppelin/contracts@4.4.2:
require(currentAllowance >= amount, "ERC20: transfer amount exceeds allowance"); unchecked { _approve(sender, _msgSender(), currentAllowance - amount); }
#0 - GalloDaSballo
2022-02-04T22:56:31Z
Am not fully confident as to why the assignment to storage would go through an oveflowcheck
Either way, because the warden identified a valid change in the different OZ versions, will mark as valid