Findings Database

The leftover transfer in BlueBundlesV1::blueBundlesV1RepayAndWithdrawCollateral() is not skipped when null, unlike the referral fee transfer above it (and on the rest of the codebase).

MidnightSupplyVaultSharesCallback::onSell() deposits the borrower's loan tokens into the configured ERC4626 vault and supplies the resulting shares as collateral, without enforcing any minimum shares received.

MidnightVaultExecutor::onLiquidate() always sends the redeemed loan tokens to the liquidator and invokes their callback only when callbackData.length 0.

mulDivDownInverse computes n = target + 1 before its overflow guard, so target == type(uint256).max reverts with arithmetic overflow instead of returning type(uint256).max as the guard's intent implies.

The n == 0 branch in mulDivDownInverse is unreachable: n is assigned target + 1, so it is always = 1 in any path that reaches the check (the only way n could be 0 is the overflow case target == type(uint256).max, which…

mulDivUpInverse computes target den via mulDivDown without an overflow guard. Since den == WAD and num == price (typically num <= den), target WAD overflows for target type(uint256).max / WAD.

IdLib.toId() includes block.chainid in the obligation id computation, so after a chain fork, all existing obligations become unreachable and interactions silently create new obligations with fresh state.

The constructor emits roleSetter (an SLOAD) in the event instead of msg.sender, wasting gas since they are the same value.

Every external function in MidnightBundles accepts midnight as a caller supplied parameter, which is not needed and could be set in the constructor instead for better phishing resistance.

EcrecoverRatifier.isRatified decodes height from the ratifier data and folds it into the EIP-712 typehash.

testReturnJumps asserts currentReturn / previousReturn ~= 1.025, but the actual ratio is ~0.975 since higher tick means higher price and lower return. The test only passes due to the 10% tolerance.

AaveETHYieldBackend uses the wrong address for Polygon's native token wrapper - it returns WETH instead of WPOL (the native token wrapper for POL/MATIC).

Aave's rounding behavior on deposits and withdrawals allows malicious users to drain accumulated yield by performing many small withdrawals, each burning more scaled balance than the actual asset value withdrawn.

AaveYieldBackend.deposit() reverts when the Aave pool is paused, frozen, or the supply cap is reached, causing a DoS for all SuperToken upgrade/mint operations.

ERC4626YieldBackend does not support ERC4626 vaults with deposit/withdrawal fees or slashing mechanisms, leading to loss of funds for other users or potential insolvency.

ERC4626YieldBackend.withdrawMax() silently succeeds when the vault is paused, leaving funds stuck after disableYieldBackend() is called.

ERC4626YieldBackend.withdrawSurplus() uses convertToAssets() to calculate the surplus, which does not account for withdrawal fees, potentially causing the function to revert or withdraw more than the actual surplus.

The skipSelfMint transient flag silently skips minting in selfMint(), which breaks contracts that extend SuperToken and expect selfMint() to always mint tokens when called.

The SDAIPriceFeed uses the instantaneous vault rate from convertToAssets() which can be manipulated through donation attacks, allowing attackers to drain the protocol.

When the stETH-USD oracle fails, WSTETHPriceFeed switches to ETHUSDxCanonical mode which returns prices in USD instead of EUR, causing incorrect price calculations.

In closeTroveFromCollateral(), the calculation trove.entireColl - params.flashLoanAmount mixes 18-decimal wWBTC with 8-decimal WBTC values, causing either transaction revert or bypassed slippage protection.

The WBTCZapper constructor approves wWBTC (wrapped WBTC) for the exchange, but the Curve pool uses WBTC directly, causing swap transactions to fail.

When users input collateral amounts that are not evenly divisible by 1e10, the WBTC to wWBTC conversion causes precision loss that may result in transaction reverts.

API3 oracles can return timestamps up to 1 hour in the future, causing an arithmetic underflow in the staleness check that temporarily DoS's the price feed.

The SDAIPriceFeed constructor does not call fetchPricePrimary() to initialize lastGoodPrice, leaving it at zero and causing potential issues on first oracle failure.

The IWBTCZapper interface has incorrect function signatures and missing functions compared to the actual WBTCZapper implementation. Several discrepancies exist between the interface and implementation: 1.

The TroveNFT contract maintains a custom ownerToTroveIds mapping that duplicates functionality already provided by the inherited ERC721Enumerable extension.

The nativeFeeProvided < totalNativeRequired check should be !=. The source bridge enforces that the native fee provided is exactly the amount + fee, link. Hence, this check is not accurate.

MetavaultsRegistry::unregisterMarketByIndex() don't check that the index matches the expected market, which may lead to issues if called twice in a row.

MetavaultsRegistry::registerChain() if called with chain id 0, works, but MetavaultsRegistry:unregisterChain() would revert with chain not found. MetavaultsRegistry:unregisterChain() reverting.

MetavaultsRegistry::removeBridgePathByIndex() doesn't exist, which could cause DoS. All other functions to remove data have a "by index" counterpart, but MetavaultsRegistry::removeBridgePath() doesn't. Possible DoS.

The structs MetavaultsRegistryState and MarketInfosState appear in different order compared to the functions and constants. Readability Manual Review Change the struct order

It's technically possible to register a zero address metavault. It would be an admin mistake. Manual Review Check if the metavault is not the zero address.

market and chainConfig shadow the corresponding functions. Market and chainConfig are used throughout the contract as memory variable, but there already exist functions with these names.

Duplicate checks MetavaultsRegistry::onlyRegistered() modifier is present on both the external functions and internal ones. Gas usage. Manual Review Keep the modifier only in the internal functions.

SpiritToken::initialize() is vulnerable to an attacker frontrunning the initialization with a malicious implementation, minting tokens for themselves, and then setting the implementation to address(0) again.

SpiritFactory::setupUniswapPool() mints a single sided position, from the current tick to the max tick. Thus, users can buy Child tokens for at least a price of the tick, which goes up as liquidity decreases.

StakingPool::unstake() reduces the units of the user pro-rata to the unstaked amount: The issue is that, as it rounds the units down, a staker is able to abuse it by unstaking many times, each time rounding it down,…

The Airstream controller is owned by the SpiritFactory, but the latter has no function to call the AirstreamController. Since the SpiritFactory is upgradeable, this can be mitigated.

The StakingPool has 1 unit so no flow is lost, but there is no way for the owner to get this Spirit back. It will flow in the next distribution, but this issue refers specifically to when there is no next distribution.

SpiritVestingFactory::createSpiritVestingContract() is as follows: Note that creating more than 1 vesting contract for the same recipient overwrites the old now.

SpiritVestingFactory::setTreasury() is single factor, so a single mistake can lose treasury access forever, which is responsible for cancelling the treasury in the SpiritVesting contract.

Usd0PP.sol::deconstruct() is currently always called with msg.sender for both receiver arguments, so separate functions to mint with flexible receiver are missing.

Usd0PP::mint() checks that the timestamp is bigger than the bond start, but Usd0PP::mintWithPermit() doesn't, which is conflicting.

SyrupBitcoinRouter::setAssetController() currently requires the asset being a COMPOSITEDEPOSITASSET, even if disabling the asset controller.

SyrupBitcoinRouter::processRedemptions() reverts if the request owner is null, so any user can frontrun the batch call and delete their request to make it revert. 1. User creates a request. 2.

SortedLinkedList::getAllValues() fetches all values from the list, which may be DoSed, both on chain and through certain rpc providers that may timeout.

SyrupBitcoinRouter::processRedemptions() transfer in each loop iteration, when the sums could be added together and transferred after the end.

SyrupBitcoinRouter::requestRedeem() checks if the asset picked can be unwrapped, but doesn't check if there actually is liquidity.

The desired feature as per the docs of setting the timelock to 0 to signal the use of the default timelock is not possible once set to per function: Contract will allow setting timelock to 0 for specific function…

GovernorTimelock::executeProposals() calls the scheduled proposal target, but only deletes the proposal information after, allowing a malicious executor to reenter and execute the same proposal more than once.

As per the Solidity docs, it's recommended to use memory safe. While we recommend to always respect Solidity's memory model, inline assembly allows you to use memory in an incompatible way.

Voter::finalize() processes in batches to allow finalizing epochs and distributing rewards without running out of gas.

Missing freeze or snapshot of auto-vote choices at epoch end allows attackers to manipulate the voting process.

RangePool::adjustToTick() breaks when the nextTick equals the current tick: However, this will desync with the Uniswap pool whenever the sqrtPrice reaches the next tick.

In DeliHook v4 pools, when a liquidity provider (LP) transfers their NFT position to a new owner, the auto-claiming of rewards is executed in favor of the new owner instead of the previous owner.

Voter::finalize() transfers the WETH, and then calls distributor.setTokensPerInterval().

A logic ordering issue in RangePool::sync causes the pool's lastUpdated timestamp to advance even when liquidity == 0.

The integer division in upsertIncentive will cause unstreamable "dust" accumulation for reward tokens as every remainder from total / WEEK is truncated.

Users can specify a price limit in their swap, limiting the amount of funds actually swapped, or there may not be enough assets of one of the tokens to swap.

DeliHookConstantProduct intends to preserve K in all swaps in order to forward fees to the FeeProcessor, and is achieved for all swap types.

DeliHook::beforeSwap() rounds down the baseFeeSpecified instead of rounding up, allowing an attacker to abuse the low gas fees on Base and the high btc price to not pay any fees (or half).

As per the ERC4626 spec, VaultV2::withdraw/redeem() don't receive a slippage parameter, which makes them vulnerable to slippage.

Due to the max rate mechanism, interest earned will be capped by the max rate.

VaultV2::accrueInterestView() rounds down when applying the perfomance and management fees, leading to reduce interest payments for the protocol.

VaultV2::setIsAdapter() removes the adapter from the array by doing a O(n) search, which is ineffective. The number of adapters is admin controlled so DoS should never happen due to OOG.

MorphoMarketV1Adapter::deallocate() withdraws the given assets, removing a market in case the allocation is null, but it may be hard to achieve this without using the exact shares amount.

VaultV2::submit() overflows here when the timelock of the selector has been set to type(uint256).max, which works as intended but could be more verbose.

ViewLogic::maxLiquidatable() computes the maximum debt to liquidate such that the resulting target health is exactly the desired: However, this is inaccurate due to the bonus, which affects the final health as it…

Although the protocol allows repayment on paused asset, it cannot be done because Lender contract tries an unnecessary (and prohibited due to pause) borrow from Vault contract.

LoopedVault::rebalance() is missing the nonReentrant modifier. LoopedVault::requestWithdraw() flow when there are not enough idle funds also never checks for reentrancy.

RebalanceLogic::aaveToMorpho() borrows from Morpho and repays Aave debt. If trying to repay more Aave debt than the available, the function will go through but borrow tokens will be left in the LoopedVault.

LoopedVault::fulfillWithdraw() transfers the asset before updating the state, breaking the CEI pattern. Transfer the assets at the end of the function. Fixed in 2a378db.

It's technically possible for the assets of the LoopedVault to decrease suddenly when requesting a withdrawal in between a vesting interval update and a black swan event.

LoopedVault::requestWithdraw() could emit an event when requesting assets, though it's possible to fetch this information by listening for shares burned. Consider adding an event. Fixed in 2a378db.

LoopedVault::maxMint() is not overriden and does not comply with ERC4626 as there is a deposit cap.

LoopedVault::requestWithdraw() burns shares as per the current totalAssets(), which uses a cached lastTotalAssets.

LoopedVault11::updateTotalAssets() doesn't set to 0 vesting interest when vestingInterestPreFee is null, so the new period will be vesting with the interest from the previous period.

RebalanceLogic::repayAavePosition() code is switched and the collateral or debt amounts intended to be dealt with will not be the actual amounts used.

All actions that instantly change the value of the vault, namely slippage from swaps, are taken into account by also reducing lastTotalAssets resulting in the next vesting amount being unaffected.

LoopedVaultV11::rebalance() doesn't have the updateTotalAssets modifier, contrarily to the rest of the functions.

RebalanceLogic::rebalanceCallback() approves morpho twice, 1st and 2nd. Remove the 2nd approval (or 1st). Fixed in ad00401.

VotingPowerProvider::registerOperator() is permissionless, so anybody can call it with any number of accounts to DoS all functions that gather operator information, such as getOperatorsAt(), getOperators().

The vault and slashers in scope of the protocol in OpNetVaultAutoDeploy are BASEVAULTVERSION and TOKENIZEDVAULTVERSION, SlasherType.INSTANT and SlasherType.VETO.

EcParing precompile always fails on certain chains due to the hardcoded gas limit, so BlsBn254 is not available on certain chains.

The non-atomic nature of setSigVerifier and commitValSetHeader will cause a potential loss of security guarantees for networks as a malicious operator can manipulate validator sets after unstaking their funds, avoiding…

A malicious operator with any voting power (considering a minimum inclusion voting power if present) smaller than the quorum can forge a proof and bypass verification, setting the next epoch header to any value, taking…

Missing getAvailableBalance() validation in FluidLocker::provideLiquidity allows staked tokens to be withdrawn after the 6 months tax free period, without every calling FluidLocker::unstake.

Pumponomics can be effectively skipped since the eth amount used to create a position on uniswap could be different than the called amount. It uses all available balance inside the contract.

The FluidEPProgramManager.cancelProgram() function incorrectly assumes that governance parameters (minimum deposit and liquidation period) remain constant throughout a program's lifecycle.

The unlock flag is meant to signal that users can unlock SUP from their Locker.

The buffer to start funding is defined as uint256 buffer = program.token.getBufferAmountByFlowRate(fundingFlowRate + subsidyFlowRate);.

Locker owners can match their SUP tokens with ETH and deploy liquidity into uniswapv3 pools, after doing this for 6 months they will get their SUP without paying tax, otherwise this tax can get up to 80%.

ShadowStrategyGauge::addVestingPosition() collects fees first, coming either from LP fees or withdrawing from the vested position.

IRamsesGauge(gauge).getReward() iterates through all periods since the first period in the Gauge, which means that whenever a new token is added, it will loop through all periods.

Reward tokens are set on Yieldoor manually, which means there is a window where rewards may be added in the Gauge, but they weren't yet added on Yieldoor.

ShadowStrategyGauge::addVestingPosition() always tries to mint liquidity, even if it is null.

ShadowStrategyGauge::collectGaugeRewards() swaps all rewards to token0 and token1 through a pool. If this pool doesn't exist or has low liquidity, it may be impossible to swap and collect rewards.

The ShadowStrategyGauge::collectGaugeRewards() will be DoSed when the swapAmount balance is 1, as it tries to swap in an amount of swapAmount / 2, which reverts.

ShadowStrategyGauge::collectGaugeRewards() receives all lower and upper tick positions in the past period.

Although at the moment there isn't an xShadow pool, so it's impossible for it to be token0 or token1, ShadowStrategyGauge::collectGaugeRewards() would exit xShadow to shadow, and then convert it back to xShadow, taking…

ManagedLeveragedVault::openDen() picks up all asset() and converts to collateral. However, some of these assets may be reserved for withdrawals, in which case the tracking would be completely off.

ManagedLeveragedVault.sol::deposit() doesn't have slippage control for the shares minted.

ManagedLeveragedVault::executeWithdrawalEpoch() repays debt to withdraw collateral from the Den to send to users, but it doesn't handle when the minimum debt limit is reached and becomes DoSed.

ManagedLeveragedVault::executeWithdrawalEpoch() asserts the new ICR is close to the old one, but the latter is never set. Marking medium as the contract is upgradeable.

ManagedLeveragedVault::increaseLeverage() will revert when the protocol is close to the max debt as it needs to borrow more than the debt allows in order for the ICR to reach the target.

ManagedLeveragedVault::decreaseLeverage() is used to increase the ICR of the Den, but it may be DoSed in case it goes below the minimum debt limit.

ManagedLeveragedVault::deposit() doesn't allow depositing before the Den is opened, which means the only way to have a minimum collateral amount to open the minimum debt in ManagedLeveragedVault::openDen() is by…

Some tokens will not work currently due to not using safeERC20. approve(), transferFrom() don't work for all tokens and safeERC20 should be used. Protocol can't be used for certain tokens.

ManagedLeveragedVault::executeWithdrawalEpoch() applies the exposure - nect swap fees loss on the users withdrawing, ending up repaying more debt than the collateral withdrawn, increasing the ICR and possibly reverting…

There are multiple unused pieces of code. See links below. Deployment costs Manual Review Remove this code.

Insertion hints depend on the ICR of the Den, on withdrawals the same hints are used in 2 different instances, which may not work and revert.

ManagedLeveragedVault::deposit() has slippage control for collVaultShares but it applies to the full deposited collateral when the user only gets a share of it due to entry fees.

ERC4626ExceededMaxRedeem first argument is owner, but the code emits it with receiver. Full function: It shows that ERC4626ExceededMaxRedeem is emitted with msg.sender, which is the receiver, not the owner.

Recovery mode doesn't allow Dens to decrease their ICR. Hence, ManagedLeveragedVault::increaseLeverage() will revert when the protocol is in Recovery mode.

ManagedLeveragedVault::increaseLeverage() allows borrowing until the maxSystemDebt limit, but the check doesn't account for the borrowing fee triggered.

ManagedLeveragedVault::donateCollateral() doesn't control slippage, leading to losses The function is: As can be seen, there is no slippage check in the coll vault, possibly leading to fund loss.

ManagedLeveragedVault::getStrategyCR(), ManagedLeveragedVault::exposurePositionWeight() and ManagedLeveragedVault:: getStrategyLeverage() may revert.

ManagedLeveragedVault::getDebtToUnwindAndCollRequested() is called on chain after the modifier claimCollateralSurplus is called, so it is updated, but offchain this modifier was not called and the view function is off.

Most ManagedLeveragedVault functions rely on totalAssets(), including ERC4626 compliant functions that must not revert; however, they will revert when there is bad debt.

Everytime a user deposits or mints, a lockup element is added to an array. An attacker can deposit on behalf of other user and fill the array with excessive elements such that it is DoSed due to OOG.

ERC4626 implements a maxRedeem() function that must specify the max amount of shares a user can redeem.

ERC4626 donations attack are mitigated by default in the current Openzeppelin implementation by setting the decimals offset to 0, which is enough to dillute attacker donations as per the assets/shares conversion, here.

The taker fee is calculated in a wrong way leading to an underestimation of the fee. The taker fee is calculated by picking up the maker amount and multiplying by (1 + fee).

UniERC20::uniTransfer() limits the gas to 5k in the solidity-utils lib version 6.3.0. See the version used in the current commit of the protocol here.

Whitelisted users pay less resolver fee but the discount is applied too early leading to rounding errors.

PreDepositVault::unearn() instantly decreases PreDepositVault::totalAssets(), punishing users that just deposited. Suppose 1000 assets and 1000 shares that some user deposited.

PreDepositVault::constructor() uses .approve(), but this doesn't work for USDT as it does not return true.

WBTC interest is disabled on Aave V3, as it is impossible to borrow. See the market here for details No profit collection for WBTC Manual Review Use an alternative yield source.

PreDepositVault has a maxDepositLimit which is not reflected in maxDeposit/Mint, breaking the ERC4626 spec.

PreDepositVault::sweep() allows sweeping aTokens, decreasing users' deposits. Same for USDS. PreDepositVault::totalAssets() is composed of asset() and aToken. Decreasing one of them leads to user losses.

The aave lending pool produces rounding errors when calling PreDepositVault::earn(). These errors were found to be negligible, being at most 1 wei, and they can either increase or decrease totalAssets() by 1.

address.transfer forwards at most 2300 gas, which means it does not work for some smart contract wallets. Admin calls PreDepositVault::sweep() using a smart contract wallet but it reverts trying to transfer native out.

RedStonePriceFeedBase::getCurrentRedStoneResponse() does not check gas left in the catch block, allowing attackers to forward just a bit of gas to make the call to chainlink revert and trigger a temporary shutdown.

StabilityPool::provideToSpOnBehalfOf() is missing the declaration in the IStabilityPool interface. Add the function to the interface and override in the implementation.

The CurveGaugeDistributor::distributeRewardsToGauge() sends the funds to curve to deposit rewards, but does not check if there is any leftover balance after doing so.

Missing priceFeedDisabled event in RedStonePriceFeedBase. Emit an event when priceFeedDisabled is set to true. Fixed in b5e5524.

CurveGaugeDistributor::distributeRewardsToGauge() calls the external contract without limiting the gas sent, which could lead to OOG in case it becomes malicious. To prevent this, ExcessivelySafeCall can be used.

When the destination reward is a contract and the selector is not 0, the function is called on the contract.

InterestRouterV2::triggerDistribution() is permissionless and may add significant rewards to the external contracts.

payWithERC20 is supposed to be used inside BlueprintV5 contract to handle payment.

createCommonProjectIDAndDeploymentRequest() is called by createAgent(), in which the user pays fees to create an agent.

createAgentWithSigWithNFT() for example signs projectId, base64RecParam, serverURL. However, it does not sign privateWorkerAddress or tokenId.

The lack of replay protection in the updateWorkerDeploymentConfigWithSig function will cause a significant loss of funds for users as a malicious actor will replay a signed transaction to repeatedly transfer funds from…

The BlueprintCore contract enforces a single deployment request per project by using a check in the deploymentRequest function: Once a worker picks up the deployment request via the submitDeploymentRequest function, the…

The resetVestingPlans check makes it impossible to increase a user's locked tokens if the increase does not push the new amount above the total unlocked tokens.

The function resetVestingPlans() is called by an administrator account and resets vesting plans for a list of users, with the corresponding amount provided as input.

Main position ticks are set according to the tick in slot0, which is not accurate if the price is near a border.

Base calculation in Leverager::isLiquidateable() is: uint256 base = owedAmount 1e18 / (vp.maxTimesLeverage - 1e18);.

Market can allow leverage higher than 2x, and this is design choice. But the check owedAmount totalDenom in isLiquidateable function doesn't allow leverage higher than 2x.

Vault::calcDeposit() calculates the shares as depositAmount bal totalSupply in the numerator. If each of these quantities has 18 decimals, this is 1e54 of precision, having only around 1e23 left.

When depositing into the leverager, if the borrowed token is not token0 or token1, the function has to perform an exactOutput swap, to receive the borrowed token0/token1.

ReserveLogic::updateIndexes() is as follows: Note that when calculating the newBorrowingIndex, it calls latestBorrowingIndex(reserve), which uses reserve.currentBorrowingRate, which was calculated at the last time the…

Strategy main position ticks are set according to: As can be seen, when the tick spacing is 1, the tick will be deemed lower sided, which adds a tick spacing (1) to the tick border.

Strategy::checkPoolActivity() reverts if the timestamp is 0. However, this will never happen.

Strategy::checkPoolActivity() is supposed to check tick delta until the tick more in the past than lookAgo.

The StrategysetSecondaryPositionsTicks function incorrectly handles negative ticks when calculating modulo, causing the secondary position to be active when it should be out-of-range and potentially leading to division…

Vault::withdraw() withdraws from the lp position whenever the idle capital is not enough. However, it withdraws too much as it does not take into account that some of the idle capital is already available.

In the withdraw function of the Leverager contract, when the borrowed asset is token1, the code uses the wrong variable in its conditional subtraction.

BoldToken::initialize() is frontrunnable. BoldToken::initialize() is called seperately after the constructor, which means that if sent in different txs (depending on deployment scripts, it may be possible to mitigate if…

When adjusting a trove, the newly minted debt is counted twice, as troveManager.getEntireSystemDebt() already includes the new debt.

tETH and XVS present blacklisting logic that could break liquidations. XVS is also pausable, possibly leading to the same issue.

Borrowers withdrawing at a loss get downside protection from cds depositors. If these cds depositors withdraw at the same price as the borrower withdrawal that triggered the loss, it works correctly.

The total cds deposited amount is decreased by the returned amount when the cds depositor withdraws, which will include any loss that the cds depositor has taken.

Cds depositors get the profit on the eth price increase up to the strike price (5%), and any increase above this threshold goes to the borrowers.

Cds depositor generates upside from borrows up to the strike price increase, but calculates the profit on the difference between the deposited and returned amounts, which only differ by the option fees.

Borrower liquidations of type 1 add a cds profit component to cds depositors by reducing totalCdsDepositedAmount by a smaller amount of this profit.

Liquidation profit is calculated in borrowingLiquidation::liquidationType1(), but is actually never handled and given to cds depositors.

borrowing::withdraw() at a loss increases the downside protected, here.

After we liquidate via liquidation type1 method, all cds owner's liquidation share will change. This will cause some liquidated collateral will be locked.

The cds amount to reduce on liquidations from each chain is given by the share of cds deposit, but then the liquidation amount by each cds depositor is given by the available liquidation amount pro-rata to the…

In the withdrawInterest function, if totalInterest is less than amount, subtracting amount from totalInterest will cause an underflow and revert the transaction due to Solidity 0.8.x's checked arithmetic, even if…

borrowing::liquidate() sends the noOfLiquidations variable as liquidation index to the other chain. However, liquidations are tracked in omniChainData.noOfLiquidations, on both chains.

borrowing::calculateCumulativeRate() does not update lastEventTime, so the cumulative rate may be increased unbounded, forcing users to repay much more debt.

BorrowLib::redeemYields() gets usdaToAbondRatioLiq, which is the amount of USDa gained from liquidations pro-rata to the total supply of abond, at current value without consideration from previous depositors.

CDSLib::withdrawUserWhoNotOptedForLiq() does not store the tax from the cds depositor profit, leaving these funds stuck.

Due to the lack of a locking mechanism and the non-atomic nature of cross-chain operations, global states may be at risk of being overwritten.

BorrowLib::calculateRatio() sets omniChainData.cdsPoolValue to previousData.totalCDSPool + netPLCdsPool, disregarding any past value of omniChainData.cdsPoolValue, when the noOfDeposits is null.

In the deposit function we update omnichain.totalAvailableLiquidationAmount based on the enw deposit, however The Withdraw function processes user withdrawals but fails to update the…

borrowLiquidation::liquidationType1() reduces omniChainData.totalCdsDepositedAmount, so the option fees will be calculated on this reduced total cds depositors, but the normalized deposit of each borrower still amounts…

Borrowing::redeemYields debits ABOND from msg.sender (which updates its state), but uses the information of the user passed to calculate the amount to be withdrawn from the external protocol to send to the user.

The protocol provides = 20% downside protection on the collateral depending on the volatility of collateral.

totalCdsDepositedAmountWithOptionFees in CDSLib::withdrawUser() is reduced by: This is incorrect as the correct amount to reduce in option fees is params.optionFees - params.optionsFeesToGetFromOtherChain, not…

The strikePrice and strikePercent are important parameters in the Borrowing::depositTokens function, which are passed to the BorrowLib::deposit function for option fee calculations.

treasury.updateYieldsFromLiquidatedLrts() updates the yield from liquidated collateral in the current chain, but this collateral could have been present in the other chain.

The signature in withDraw can be reused and this will cause users can choose one improper odosAssembledData and convert less collateral than expected. In borrowing.sol:withDraw, borrowers can withdraw their collateral.

excessProfitCumulativeValue in withdraw() can be manipulated. Malicious users can manipulate this excessProfitCumulativeValue to withdraw more than expected.

In CDS.sol updateDownsideProtected() has no access control so malicious users can set downsideProtected to a large value which will DOS the system.

borrowingLiquidation::liquidationType1() reduces omniChainData.totalVolumeOfBorrowersAmountinWei and omniChainData.totalCdsDepositedAmount without any consideration for the cumulative values so far, leading to losses.

borrowingLiquidation::liquidationType1() withdraws the bond from the external protocol and updates the interest, but does not check if there is bond supply to distribute the interest to.

GlobalVariables::oftOrCollateralReceiveFromOtherChains() send a message to the other chain requesting token/eth transfers to the current chain.

GlobalVariables::oftOrCollateralReceiveFromOtherChains() frequently occurs losses by forwarding too much ETH as it assumes collateral transfer always happen for the 2 tokens + eth, which is not true as it can just be…

During borrower liquidation (liquidationType1()) and withdrawal from the CDS, the yield from LRTs is updated and stored in the treasury.

An underflow in cds profits computation can cause liquidation type 1 to revert. Here is the formula for cds profits computation.

The borrowing::withdraw is used to withdraw collateral from the protocol and burn USDa.

Treasury.noOfBorrowers is increased when a user deposits for the first time, as the index is 0.

CDSLib::calculateCumulativeRate() adds option fees to totalCdsDepositedAmountWithOptionFees if totalCdsDepositedAmount is not null, skipping if it is null.

The depositTokens() and withdraw() functions in the Borrowing contract exhibit inconsistent use of the lastCumulativeRate.

During liquidation only record yield accrued from IONIC, however those yields are not utilized/there is no way to withdraw them.

Missing update of lastEthprice in borrowing.soldepositTokens() will cause manipulation of omniChainData.cdsPoolValue as an attaker replays borrowing.soldepositTokens() by breaking protocol.

LoopStrategy::onMoreFlashLoan() does not validate that the caller is the market, which allows attackers to steal collateral by calling it directly. Revert if the caller is not markets. Addressed in 7d71809.

path is not validated in redeem/withdraw(), which could lead to problems as the flow should always sell and buy the same token.

The market accrues interest, which changes totalAssets() in theLoopStrategy. Thus, everytime there is some action that modifies the amount of assets, interest should always be accrued first.

LoopStrategy::maxDeposit() does not consider the vault cap, which limits the amount that can be deposited if the utilization rate is close to the targetUtilization. Hence, deposits will revert.

LoopStrategy inherits ERC4626Upgradeable, which calculates shares deposited as As such, the following attack is possible: - User deposits 1e18 assets.

LoopStrategy::onMoreFlashLoan() underflows when cost collateralToWithdraw, which may happen in case the market has a high ltv and significant slippage occurs. Consider using the wFlow from the vault in this case.

The LoopStrategy is not supposed to hold wFlow directly, but this is still tracked in totalAssets(). However, this is not redeemed in withdraw(), so it will be stuck.

In LoopStrategy::withdraw(), updateLastTotalAssets() is called after withdraw(). Follow the CEI pattern and update the total assets before the withdraw() call. Addressed in c4b4f1a.

LoopStrategy::deposit() stops iterating if the utilization ratio reaches exactly the target, that is, market.totalSupplyAssets.wMulDown(targetUtilization) == totalBorrowAssets.

LoopStrategy::maxDeposit() has as a comment totolSuppliable, which has a typo. Replace totolSuppliable with totalSuppliable. Addressed in 40132e8.

LoopStrategy::onMoreFlashLoan() does not reset the approval of the markets to 0 after approving the repayment of wFlow.

Some tokens do not return a bool on approval, so safeApprove() should be used. Consider replacing all instances of approve() by safeApprove(). Can be kept as is as the current tokens work without issues.

paramsOfTheMarket is unused in the LoopStrategy. Remove or deprecate the variable (comment or rename it). Addressed in 50a9248.

MapleSkyStrategy:: gemForUsds() computes gemAmount = (usdsAmount WAD) / (to18ConversionFactor (WAD + tout));. As can be seen, to18ConversionFactor is 1e12 for a Usdc gem, which leads to a rounding up to 1e12 Usds.

Withdrawing all the Aave or Usds balance from the strategy may revert when there is yield.

MapleSkyStrategy::gemForUsds() and MapleSkyStrategy::usdsForGem() do not cache the psm nor add trailing underscores to tout and to18ConversionFactor. Implement the fixes to ensure gas savings and correct formats.

The MapleAaveStrategyStorage contract is missing the State Variables separator comment that is present in other similar contracts like MapleSkyStrategyStorage and MapleBasicStrategyStorage.

The EIP4626 interface says for previewRedeem() that it may revert, which will DoS withdrawals in the MaplePool: MAY revert due to other conditions that would also cause redeem to revert.

MapleBasicStrategy::withdrawFromStrategy() does not send an upper limit on the amount of shares minted, which means an unexpected loss may happen in case of slashing before the withdrawal from the EIR4626 vault.

MapleSkyStrategy::assetsUnderManagement() uses maxWithdraw(), which according to EIP4626 returns 0 when the withdrawal limit is reached or similar.

MapleSkyStrategy::setPsm() changes Psm, but does not set the old Psm approval to 0. Set the old Psm approval to 0 as it is no longer needed. Addressed in 6c9f85d.

Currently tin is null in the Psm, but if it was non null, it would instantly lead to a reduction in the totalAssets() in the Maple Pool, as a fee would be taken when converting fundsAssets to Usds in the Sky Strategy,…

The psm contract may not have enough gem (USDC) in the pocket, which could revert and DoS withdrawals. It's possible to change the psm address so the issue can be managed, but it could DoS withdrawals for some time.

In the psmWrapper, the Usds amount from the gem is calculated as usdsInWad = gemAmt18 + gemAmt18 psm.tout() / WAD; , but in the Sky Strategy it is calculated as (gemAmount to18ConversionFactor (WAD + tout)) / WAD; .

The daiJoin contract may be caged, which means exit() is DoSed forever.

The DssLitePsm, used as part of the Usds Psm wrapper that allows converting fundsAsset to Usds in the Sky Strategy, may be halted by setting tout to type(uint256).max.

The Aave strategy does not deal with the reward controller, so it could miss out on rewards emitted in case the fundsAsset is listed in the RewardsController.

When assetsUnderManagement() decreases, a window of opportunity is created for people to stake in the maple pool very cheaply and then sell for profit when it becomes active again or the protocol admin calls…

FluidLocker::getUnlockingPercentage() calculates the percentage to unlock, which is the amount given to the user, while the remaining goes to other stakers.

FluidLocker::getUnlockingPercentage() calculates the amount to unlock when unvesting via the FluidLocker.

Superfluid flows reserve 4 hours of the stream flow rate as a buffer for liquidations, and returns this when the flow is closed.

FluidLocker::claim() connects to the fluid pool and then calls EPProgramManager::batchUpdateUserUnits() to verify the signature and update user points.

FluidLocker::getUnlockingPercentage() is calculated as: As can be seen, it divides before multiplying, leading to precision loss.

The missing check in FluidLocker::instantUnlock() for stakers in the tax pool allows users to unlock all their funds without paying any tax instantly.

BancorExchangeProvider::getScaledAmountOut() decreases the amount out when the token in is the supply token by the exit contribution, that is, scaledAmountOut = (scaledAmountOut (MAXWEIGHT - exchange.exitContribution))…

GoodDollarExchangeProvider::mintFromExpansion() mints supply tokens while keeping the current price constant.

GoodDollarExpansionController::mintUBIFromReserveBalance() or GoodDollarExpansionController::mintUBIFromInterest() transfer funds to the reserve and mint $G to the distribution helper.

TradingLimits::update() divides the traded funds by the decimals of the token, int256 deltaFlowUnits = deltaFlow / int256((10 uint256(decimals)));.

numberOfExpansions = (block.timestamp - config.lastExpansion) / config.expansionFrequency The calculation divides it by the expansionFrequency, but this will cause significant rounding issues.

Users may upgrade their towers so they receive gameETH over time which can later be withdrawn for ETH.

In Codeup::addGameETH() and Codeup::reinvest(), gameETH is obtained as gameETH = amount / gameETHPrice, which means there may be rounding errors.

Codeup::withdraw() and Codeup::reinvest() cap the withdrawn amount to the available ETH balance, but always set the tower.gameETHForWithdraw to 0, which means that a user can have a significant amont of gameETH for…

Uniswap pools may be created without the underlying tokens existence, which means that someone may do this before Codeup::claimCodeupERC20() is ever called, making it revert when UniswapV2Factory::createPair() is called…

Codeup::claimCodeupERC20() adds liquidity to the Uniswap pool whenever the weth balance is bigger than 1. However, an amount bigger than 1 may still lead to reverts if it is low enough.

gameETHForWithdrawRate is set in Codeup::constructor() to gameETHPrice / 1000 but a zero check is missing. Add the checkValue() function to gameETHForWithdrawRate = checkValue(gameETHPrice / 1000).

Codeup::claimCodeupERC20() does not set minimum values for adding liquidity or swapping (sets 0) and places a deadline of block.timestamp, which means mev bots may sandwich these calls for profit for the protocol's…

There are typos in the codebase that could be fixed. The following instances were found: availble, spend, luqidity, spended, emitted (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11), hrs (should be mins, 1, 2), totalInvestedBedore.

Hardcoded values should be avoided for better readability. The following instances were found: (tokenAmount 10) / 100 (1, 2), Codeup::syncTower(), Codeup::getUpgradePrice(), Codeup::getYield()

Only Codeup::addGameETH() checks if the protocol has started in require(block.timestamp startUNIX, NotStarted());, but the same checks could be applied to the remaining functions.

CodeupERC20 inherits Ownable but does not use any of its functionalities. Remove Ownable as it is not used.

The TransferFailed error in CodeupERC20 is not used and can be removed. Remove this error.

MellowPriceFeed gets the ratio by doing answer = int256(uint256(ratiosX96[0])) 1e18 / int256(managedRatiosOracle.Q96());, hardcoding the decimals to 1e18.

The price is calculated based on the ratio from managedRatiosOracle.getTargetRatiosX96(). Currently the vault has only 1 underlying token, which means the ratio will always be 1e18.

totalEarnings in LidoVault::vaultEndedWithdraw() is calculated as: The withdrawn shares are scaled to get the total earnings, along with vaultEndedStakingEarnings, which was aquired by getting the liquidity from the…

The incorrect accounting of protocol fee will cause double charging fee and wrong distribution of earnings for variable users.

When FIXED users deposit ETH, they are being deposited in Lido, and Lido might experience slashing.

In LidoVault::withdraw(), when the vault has started but not ended, it limits the value to withdraw if a slashing event occured and withdraws lidoStETHBalance.mulDiv(fixedBearerToken[msg.sender],…

The parameters of each LidoVault are tuned such that fixed depositors get an upfront premium and variable depositors in return get all the yield produxed by the fixed depositors' deposits.

will change the but after this, will assume that these has been the same as the starting ones and will incorrectly compound them. A user can create a by calling , deposit his token and take back an amount as debt.

Lister unfairly double pays the while he is cancelling his through . When a user is creating his through , he is paying upfront the tax that is expected to be used for the whole duration of the .

In the unlockProtectedListing() function, the interest that was supposed to be distributed to LP holders was instead burned.

There is no slippage protection while removing liquidity and swap tokens from AMM. There are 2 intances where slippage protection is missing which are as below: 1.

VaultLib::redeemEarly() is called when users redeem early via Vault::redeemEarlyLv(), which allows users to redeem Lv for Ra and pay a fee.

VaultPoolLib::reserve() stores the Pa attributed to withdrawals in self.withdrawalPool.stagnatedPaBalance instead of storing the amount attributedToAmm.

The protocol deposits RA and CT tokens to an AMM pair, from fees or when users call the depositLv() function.

When Liquidating LP , DS and CT are paired , then that amount is used to redeem RA . But the accounting for RA has been done incorrectly since it does not account for exchange rate.

repurchase() function take redemption asset and gives back depeg swap along with pegged.

Vault::redeemExpiredLv() calls VaultLib::redeemExpired(), which allows users to withdraw funds after expiry, even if they have not requested a redemption.

Users have the option to repurchase DS + PA by providing RA to the PSM. A portion of the RA provided is taken as a fee, and this fee is used to mint CT + DS for providing liquidity to the AMM pair.

VaultLib::liquidatedLp() calls PsmLib::lvRedeemRaWithCtDs(), which redeems ra with ct and ds.

FlashSwapRouter::swapDsforRa() is called as part of FlashSwapRouter::swapRaforDs() whenever the reserve is sold and the resulting Ra is used to provide liquidity to the Vault by calling…

The modifier LVDepositNotPaused in Vault::depositLv() checks [states[id].vault.config.isWithdrawalPaused]( instead of [states[id].vault.config.isDepositPaused]( which means deposits will only be paused if withdrawals…

The AssetFactory and FlashSwapRouter inherit the UUPSUpgradeable contract in order to be upgradeable.

VaultLib:redeemEarly() redeems users' liquidity vault positions, lv for Ra, before expiry. After expiry, it is not possible to deposit into the vault or redeem early.

The readme states that rebasing tokens are supported Rebasing tokens are supported with exchange rate mechanism However, only non rebasing tokens such as the wrapped version wsteth are supposed.

When providing liquidity to an AMM pair, the protocol specifies both the desired amount of tokens to be provided and a minimum amount to be accepted.

SyrupUserActions sets a minimum amount out limit to prevent excessive MEV. However, as the deadline parameter is set to block.timestamp there is still some room for MEV.

Some extra spaces were found in the codebase which could be fixed for increased readability. - bytes32 public immutable override poolId; in SyrupUserActions. - // 4.

SyrupUserActions::swap() checks the slippage at the end require(amountOut = minAmountOut, "SUA:S:INSUFFICIENTAMOUNTOUT");.

The permit() function, utilized in the MplUserActions, SyrupRouter, and SyrupUserActions contracts, is duplicated across them.

The following immutable variables are unnecessarily included in event emissions: - SyrupUserActions::swap() - syrupUsdc in Swap event.

The WinnablesTicketManager::propagateRaffleWinner function is vulnerable to misuse, where incorrect CCIP inputs can lead to assets being permanently locked in the WinnablesPrizeManager contract.

The WinnablesTicketManager::cancelRaffle function is vulnerable to abuse because it is an external function that allows anyone to cancel a raffle if its status is set to PRIZELOCKED.

The variable lockedETH keeps track of the ETH(AVAX) collected by the raffles that are underway. The owner can't withdraw this amount.

A core invariant defined in the contest README is that: Admins cannot affect the odds of a raffle While the centralization risk of admin self-minting tickets is noted, the following assumption is noted: The existence of…

By whitelisting their address and sending a WINNERDRAWN CCIP message to PrizeManager, Admin can steal the raffle prize, which breaks the invariant from README: Winnables admins cannot do anything to prevent a winner…

WinnablesTicket stores nft ownership by setting the first minted nft id ownership to the user minting and all the next minted nfts remain as 0.

MultiplierContract::proposeMultipleUpdates() updates maxLevel without going through the UPDATEINTERVAL, which will affect the multiplier MultiplierContract::getMultiplier() right away, without updating the actual…

IdoTokens are allocated in IDOPoolAbstract::enableIDORound() up to idoConfig.idoSize.

Rounds may be claimed after finalized, in which each claim reduces globalTokenAllocPerIDORound[idoConfig.idoToken].

IDOPoolAbstract::adminAddRegForMetaIDO() sets metaIDO.isRegistered[user] = true;, but not the rank and multiplier.

IDOPoolAbstract::withdrawSpareIDO() checks if contractBal = ido.idoSize, where contractBal = IERC20(ido.idoToken).balanceOf(address(this)).

The fyToken contribution limit is enforced as: As can be seen, maxFyTokenFunding is in idoToken units (idoSize refers to idoToken) and globalTotalFunded in fyToken or buyToken units.

Blast accumulates yield for contracts holding USDB, WETH and ETH and enables contracts to claim some of the gas fees. However, this is not currently dealt with so IDOPoolAbstract will miss out on this.

The refund event is emitted as emit RefundClaim(idoRoundId, msg.sender, pos.amount, pos.fyAmount);. pos is defined as IDOStructs.Position storage pos = idoConfig.accountPositions[msg.sender];.

IDOPoolAbstract::manageRoundToMetaIDO() does not check if idoRoundClocks[roundId].parentMetaIdoId has already been set, allowing it to be overriden and rounds to be linked to more than 1 metaIDO in the metaIDO.roundIds…

address.transfer is unsafe and may lead to stuck tokens as it forwards 2300 gas, which may run out of gas in the fallback function of the receiver. Additionally, future opcode changes could make it stop working.

IDOPoolAbstract::depositToTreasury() transfers pos.fyAmount and pos.amount - pos.fyAmount, but one of them may be 0 if the account only used buyTokens or fyTokens.

IDORoundConfig.idoPrice is used when converting from IdoToken to buyToken or fyToken, but there is no indication of the code of its units.

The implementation contract StandardIDOPool can be initialized by an attacker, although it can not do anything with it. Call disableInitializers() in the constructor of StandardIDOPool.

StakedEXA::update() is called when minting and if the time staked is smaller than the reference time, it calls claim(reward), which claims the rewards for the msg.sender.

Market::liquidate() gets the maximum assets to liquidate from Auditor::checkLiquidation().

This issue remains from the last audit, only a partial fix was applied.

In Market::clearBadDebt(), it only clears bad debt if the earningsAccumulator is bigger than the bad debt to clear, but the fixed pool in question may have unassigned earnings which would be added to the earnings…

In the StakedEXA contract, the notifyRewardAmount function suffers from precision loss when calculating the reward rate, leading to some rewards being locked and unclaimable.

StakedExa::update() resets avgStart[to] whenever the time passed is bigger than the reference time.

StakedEXA::harvest() withdraws from the market of the StakedEXA contract if the provider has assets there and has approved StakedEXA.

In StakedEXA::harvest(), it deposits to the market the resulting savings that are not notified as rewards from the provider.

StakedEXA::update() harvests when depositing, withdrawing assets from the provider in the given market.

Rewards are destributed via StakedEXA::notifyRewardAmount() over a certain duration and amount, which originates a rate.

The provider in StakedEXA is free to distributed rewards as it pleases and there is an automated way to distribute via StakedEXA::harvest().

Market::liquidate() calculates the amount to liquidate in Auditor::checkLiquidation(), which does not take into account floating assets accrual due to unassigned earnings in Market::noTransferRepayAtMaturity().

Vault::withdraw() transfers funds to an user if address(this)).balance = amount, which may transfer funds resulting from fees.

Using address.transfer is not recommended as future gas cost changes can make it no longer work or users might use smart contract wallets.

In BRC20Factory::burn(), Vault::deposit(), it is required that the fee is equal to or exceeds the msg.value.

It's recommended to change owner using a 2 step procedure to mitigate the risk of the contract losing ownership due to incorrect address.

BRC20Factory::mint(), BRC20Factory::burn(), Vault::deposit() and Vault::withdraw() allow sending any token, which could lead to problems if the token is not a BRC20. Validate the sent token was created by the factory.

BRC20Factory::burn() does not validate the receiver, which could lead to wasted funds. It's impossible to do a fail safe verification, but at least the length could be checked.

The signature verification in BRC20Factory and Vault does not check that the recovered address is not null, which means that if address(0) is added as a signer, it would be possible to forge signatures.

The fee is set in the constructors of BRC20Factory and Vault, but the event is not emitted. Emit the FeeChanged event in the constructor of both contracts. Addressed in edbf47e.

BRC20Factory constructor does not check for duplicate signers, which would lead to problems as the index would be overriden. Check for duplicate signers using the recommendation from 4. Addressed in ea46e07.

The domain separator is cached in the constructor of the contracts, which could lead to signature signing softwares in case of a hard fork.

State changes should always be performed before interacting with an external contract, or it increases the risk of reentrancy attacks. In Vault::withdraw() decrease allowances[token] before sending the funds.

BRC20Factory checks if the account is authorized and if its index stored in indexes[account] is smaller than the length of signers.

Reading/writing from the same storage variable more than once should be avoided to save gas. BRC20Factory::burn() can cache the fee (read twice). Vault::deposit() cache the fee (read thrice).

BRC20Factory::createBRC20() creates new BRC20 tokens by setting the parameters storage variable to the name, symbol and decimals of the new token, and then the BRC20 token in the constructor fetching this storage…

Signers are checked for duplicates by creating a memory signers array and looping through this array for every new signer, which is inefficient.

BRC20Factory and Vault store the signers array, the authorized and indexes mapping.

Storing a uint256 instead of a bool saves gas as it is cheaper changing storage from non null to non null value. Implement the guard with a uint256. Here is an example implementation from OpenZeppelin.

KeyringCoreV2::createCredential() downcasts several variables, uint24(policyId), uint32(epoch), uint32(epochExp), uint168(cost) when sending them to RsaVerifyOptimized::verifyAuthMessage(), where they are encoded to a…

RSA signatures rely on the size of the key for higher safety assurances. As computational power advances, it becomes possible to brute force signatures up to more bits.

RsaVerifyOptimized::pkcs1Sha256() code has slight changes to the original code.

address.transfer() hardcodes 2300 gas to forward, which may not be enough if gas costs change in the future or to is a smart contract wallet that does some logic in its receive()/fallback() function.

When setting the admin in KeyringCoreV2Base::setAdmin() and the constructor an event should be emitted. Emit events on all relevant state variable changes. Addressed in ffe8b11.

Admin transfer in KeyringCoreV2Base is performed at once, possibly setting the admin to an incorrect address.

KeyringCoreV2Base::createCredential() checks that the current block.timestamp has not exceeded creatBefore by doing: It could use currentTime instead as it has been cached. Addressed in ffe8b11.

KeyringCoreV2Base::blacklistEntity() and KeyringCoreV2Base::unblacklistEntity() could perform additional checks such that the address is not already blacklisted or not.

Usually access control is performed in modifiers, as is the case with onlyOwner, for example.

KeyringCoreV2Base::registerKey() registers a key with a validity between validFrom and validTo. It checks if validTo <= validFrom, but does not confirm that validTo is not set in the past. Addressed in ffe8b11.

Functions that are never called internally should be marked external. KeyringCoreV2Base::collectFees() to external. Addressed in ffe8b11.

EntityData and KeyEntry structs have a natspec parameter named PADDING which is not present. Remove the PADDING parameters from the comments. Addressed in ffe8b11.

BasicVault::redeem() is inconsistent as it burns newAmount, the value returned from the hook, but pushes a request to the redeem queue with amount.

The deposit and redeem functions of the vault take an amount parameter and mint/burn the corresponding amounts assuming a 1:1 ratio.

The gas limit of messages must be set in the metadata function, as indicated by the docs. When it is not set, the gas limit defaults to 50000, which may not be enough to handle the epoch increase on the destination.

StrategyExecutor::disableStrategy() removes a strategy from the enabled array by moving the last element to the to be deleted element and popping.

RedeemQueue::getAvailableResolveRange() returns the indices to resolve a requestor, but returns nothing if from == count - 1.

BasicVault::redeem() only calls BasicVault::resolveWithIdleBalance() when the redeem queue is enabled, but it should also do it when it is disabled as not enough assets may have been reserved.

BasicVault::deposit() only calls BasicVault::resolveWithIdleBalance() if the redeem queue is enabled;

Contracts inheriting Ownable2StepUpgradeable or Ownable allow for renouncing ownership, which could be catastrophic if triggered by mistake.

The StrategyExecutor contract inherits Ownable2StepUpgradeable and PausableUpgradeable, allowing the owner to pause the contract.

BasicVault::manualDeposit() does not pull assets from the msg.sender, but BasicVault::manualRedeem() burns shares from msg.sender, which is inconsistent.

In BasicVault::getRedeemRequestOf(), redeemRequests is filled with the requests from the queue from the IndexByRequestor struct offset, which stores the last non resolved request.

Cap::receive() has no restrictions in place of the msg.sender, which means anyone can send funds to Cap, without any effect. Delete the function or specify who may call it. Addressed in b9ed378.

The code is using OpenZeppelin's Context contract which is intended to allow meta-transactions.

To enhance code clarity, consider employing named mappings. Addressed in dfc8c2c.

Linea does not support PUSH0, so it may be necessary to compile the contracts with an old em version (such as Paris, which is the one currently used).

BasicVaultFactory::createVault() reverts with a vault already created error whenever $.vaultIds[vault] != 0 && $.vaults[$.vaultIds[vault]] != vault.

When locking the implementation contract, the usual behaviour is calling disableInitializers().

pragma abicoder v2; is not required anymore as it is used by default. Remove pragma abicoder v2;. Addressed in 8b05c08.

resolvedCount in RedeemQueue::isResolved() always increments in the loop, regardless of it being resolved or not. The function is not exposed so it has no impact but could be a future issue.

RedeemQueue::findOffsetIndex() performs binary search but does not check the last index when doing so, as it assigns uint256 r = len - 1;.

The style guide can be found here, and structs should not be placed in between code. Follow the style guide for better readability. New Issue

The current RedeemQueue implementation goes through requests and marks them as resolved one by one.

RedeemQueue::get() reverts due to underflow if index.count == 0 instead of the error IndexOutOfRange. IndexOutOfRange(index.offset, index.count, idx); Addressed in 17568a7.

BasicVault::deposit() checks if amount is negative, which is unnecessary. Replace <= by ==. Addressed in b13e220.

BasicVault::resolveWithIdleBalance() returns when there is enough reserved amount to fulfill all requests. In this case, there is no need to fetch the idleBalance(), should the order should be inverted to save gas.

Storage variables should be cached to avoid reading from storage more than once and incurring extra gas costs. In BasicVault::enableEOL(), $.asset.forceApprove(address($v2.eol), type(uint256).max);

BasicVault is initialized by calling asset::decimals(), but as it is not part of the standard, it may not be implement the function in the expected way or at all.

Cap::checkRemoteStateAndAdvance() advances the next epoch if all domains are equal to the minimum domain found. Thus, if any of the domains is not equal to the minimum, it will return anyway.

Cap::checkRemoteStateAndAdvance() returns if not all domains have the same epoch, but the domain with index 0 is skipped in the loop.

In Cap::processDone(), it returns if no domains are set, if (domains.length == 0) return;.

Cap::setEpochCap() reverts when nextCap < $.cap[i], allowing the next cap to be equal to the current cap, which does not seem intended according to the error reason 'cap should be greater than previous cap'.

In AggregateHook::reportRedeem(), AggregateHook::previewReportClaim() and AggregateHook::reportClaim(), it sets newAmount to prevAmount if newAmount is null, which is always, as it is initialized by default to null.

Mismatch between the return amount in all yield Etherfi and Lido assets and the getAccountYieldBalance() call causes the yield amount of each staked nft to the same YieldAccount to be incorrect, leading to the…

Bad debt is never handled, which may happen whenever the collateral asset price crashes or the debt value spikes.

In YiedlETHStakingEtherfi::stake, users yieldBorrow WETH, convert it to ETH, and stake in Ether.fi's LiquidityPool. Ether.fi's LiquidityPool will mint eETH (protocol token) to the user's yieldAccount.

In IsolateLogic.sol, liquidation of an isolate loan can only be placed after the auction period is passed with bidding.

The owner of Etherfi has the ability to invalidate withdrawal requests, which will DoS YieldEthStakingEtherfi::protocolClaimWithdraw() without ever repaying the debt.

Whenever the price of a nft suffers a significant drop or the debt asset increases in price, there is a threshold after which liquidations are not profitable at all for the liquidator.

No means to retrieve collected fines from the staking contract.

User may revoke the underlying asset approval from the yield contract and prevent the botAdmin from repaying and closing his position. As a result, the debt will not be repaid, leaving the protocol at a loss.

FairLauncher::claims() reverts if the ETH claimed from overfunded presales fails. However, this means that Ole and D1MemeToken claiming will also revert, when this could be prevented.

Users receive discounted shares when they participate in FairLauncher::participate() but the amount raised has exceeded the hard cap set.

Blast earns yield for addresses that hold ETH. The FairLauncher contract will hold ETH due to the sales, but inherits BlastNoYieldAdapter, which does not configure the Blast address yield to claimable.

The issuerAddress in the FairLauncher and TokenVault is not set in the constructor and the signature libraries do not check if ecrecover returns address(0).

The gasOperator or feesCfg may not be set when users create a new fair launch, as they are not set in the constructor and FairLauncher::newFairLaunch() is permissionless, so users may snipe the creation of a new fair…

Erc20Utils::safeTransferFrom() checks the balance of address(this) before and after transferring, but it may transfer the tokens to a different address, in which case it would always return 0.

The deadline parameter to swap Ole is block.timestamp, so there is some room for MEV, although this is technically not possible on Blast as there is not a public mempool. Send the deadline as a parameter. Acknowledged

In FairLauncher::calInviteFees(), state updates are skipped if the directInviter is address(0), but they could also be skipped if the amount is null, which happens after the hard cap is reached.

In FairLauncher::newFairLaunch(), the total supply of minted D1MemeToken is equal to the presale + lp + freeClaim + airdrop.

TokenVault does not track or set the withdrawal logic on chain, so care must be taken in the offchain components due to reorgs.

SyrupRouter::depositWithPermit() accepts a permit signature and a depositData as extra data to be emitted.

SyrupRouter::depositWithPermit() uses the permit functionality of the asset by calling IERC20Like(asset).permit(owner, address(this), amount, deadline, v, r, s);.

EIP712 defines the digest as keccak256("\x19\x01" ‖ domainSeparator ‖ hashStruct(message)). domainSeparator is keccak256(abi.encode(TYPEHASH, hashedName, hashedVersion, block.chainid, address(this))).

SyrupRateProvider hardcodes the shares when calling IPoolLike(pool).convertToExitAssets(1e18);. Consider using a constant state value instead along with a comment for better readability. Addressed in 32b7bc3

SyrupRouter::deposit() checks for owner permission to deposit, but due to transferring the shares later in ERC20Helper::transfer(), it also requires P:transfer permission.

The vault portion calculation in PrizePool::getVaultPortion() is incorrect because it fetches a startDrawIdInclusive that has been overwritten in the total accumulator but not in the vaults accumulator or the donation…

The sum of the calculated startDraw and finishDraw reward fractions likely exceeds 1.0, which can lead to overpaying rewards or an InsufficientReserve error.

Estimated prize draws in TieredLiquidityDistributor are off due to rounding down when calculating the expected prize count on each tier, leading to an incorrect next number of tiers and distribution of rewards.

If a claimer propose an array of prizes to claim, but some of these prizes have already been claimed, or some claims revert, then the actual feePerClaim received will be less compared to what it should really be, as…

Witnet is not deployed on Blast, Linea, ZkSync, Zerion, so draws will not be possible there. Vaults can still be deployed and yield earned, so it will fail when trying to start or complete draws.

Inconsistent checks in the DrawManager.canStartDraw function, neglecting to consider retried RNG requests, might lead to wrongly assuming that a new draw auction cannot be started.

maxDeposit doesn't comply with ERC-4626 since depositing the returned amount can cause reverts The contract's maxDeposit function doesn't comply with ERC-4626 which is a mentioned requirement.

A malicious winner can exploit the claimPrizes function in the Claimer contract by reverting the transaction through returning a huge data chunk.

The linearly decreasing auction formula in TpdaLiquidationPair.computePrice() does not account for sudden increases in the vault's liquidatable balance, causing the price returned to be much lower than it should be.

The user calls claimPrizes to collect prizes for the winner, earning some rewards in the process. However, during this process, the winner can steal the fees collected by the user without paying any gas fees.

- The Requestor contract uses transfer to send ETH which has the risk that it will not work if the gas cost increases/decrease(low Likelihood), but it is highly likely to fail on zkSync due to gas limits.

PrizeVault::liquidatableBalanceOf() is called in TpdaLiquidationPair::availableBalance() to get the maximum amount to liquidate, which will be incorrect when tokenOut is the asset of the PrizeVault, due to not taking…

Buffer unexpected delay due to sequencer outage is inconsistent. When the sequencer is down, users may call SequencerInbox::forceInclusion() to get their message added to the inbox sequencerInboxAccs.

The OperatorDelegator.completeQueuedWithdrawal() function is used by admins to finalize previously initiated withdraws of shares from EigenLayer.

Deposit and withdrawal requests can be done immediately with no costs or fees, and both use the current oracle prices and TVL calculation (deposit, and withdraw).

When the OperatorDelegator::completeQueuedWithdrawal function is invoked to finalize a withdrawal from EL, it attempts to utilize the accumulated ERC20 tokens to fill the ERC20 withdrawal buffer, as demonstrated in the…

When calculating TVL it iterates over all the operator delegators and inside it iterates over all the collateral tokens.

The OperatorDelegator.completeQueuedWithdrawal() function serves to finalize a queued withdrawal consisting of different tokens, sending the withdrawn tokens to the WithdrawQueue contract up to the buffer amount, and…

Administrator is not able to pause users' withdrawals and claims as expected. The WithdrawQueue contract inherits PausableUpgradable to provide pausing capabilities to the administrator on users' withdrawals and claims.

LenderCommitmentGroup (LCG) will have its funds stuck if transferFrom inside sendOrEscrowFunds reverts for some reason. This will increase the share price but not transfer any funds, causing insolvency.

The burnSharesToWithdrawEarnings function burns shares before calculating the share price, resulting in an increase in share value and causing users to be overpaid.

liquidateDefaultedLoanWithIncentive sends the collateral to the Lender - LenderCommitmentGroup (LCG) instead of the liquidator. Liquidators will not be incentivized to liquidate.

Anyone can steal pool shares from lender group if no-revert-on-failure tokens are used.

LenderCommitmentGroupSmart::acceptFundsForAcceptBid() multiplies collateralAmount by STANDARDEXPANSIONFACTOR (1e18), allowing users to borrow with 1e18 times less collateral.

LenderCommitmentGroupSmart calculates the spot and twap prices of the Uniswap pool and ideally picks the worst price for the user, but this is not the case and the opposite is true.

LenderCommitmentGroupSmart gets the interest directly from the utilization ratio, which may be gamed and a loan may be taken with lower interest rate at no risk.

Issue 497 from the previous Sherlock audit was not fixed in the current code and is still present.

FlashRolloverLoanG5::acceptCommitment() allows picking the SmartCommitmentForwarder, but the selector is incorrect, making it unusable for LenderCommitmentGroupSmart.

FlashRolloverLoanG5 calls SmartCommitmentForwarder::acceptCommitmentWithRecipient(), which will have CollateralManager commiting tokens from FlashRolloverLoanG5, which will revert as it does not approve it nor have the…

FlashRolloverLoanG5::repayLoanFull() approves TELLERV2 for repayAmount, but TELLERV2 always pulls the principal and interest, possibly leaving some dust approval left.

The getPriceFromSqrtX96 will revert for pools that return a sqrtPriceX96 bigger than type(uint128).max.

Ownableinit() is not called in LenderCommitmentGroupSmart::initialize(), which will make the contract not have any owner.

LenderCommitmentGroupSmart calculates the exchange rate and valueOfUnderlying() without using mulDiv from OpenZeppelin, which might make it overflow, leading to DoS and possible loss of funds.

LenderCommitmentGroupSmarttest::addPrincipalToCommitmentGroup() and LenderCommitmentGroupSmarttest::burnSharesToWithdrawEarnings() are vulnerable to slippage attacks and should set slippage protection, contrarily to the…

LenderCommitmentGroup (LCG) calculates the APR borrowers can borrow on using getMinInterestRate. However, this math doesn't include the amount that the borrower is currently borrowing.

LenderCommitGroupSmart contract incorporates internal accounting for the amount of tokens deposited, withdrawn, etc. The problem is that if one of the pools has a fee-on-transfer token, the accounting is not adjusted.

Users can exploit the batch minting feature to avoid paying minting fees for tokens, leading to a loss of fees for the creator and fee recipients.

Collection referrers will not receive their share of the minting fee, leading to a loss of assets for the collection referrers.

The signature in TitleGraph.acknowledgeEdge() and TitleGraph.unacknowledgeEdge() is generated based on a digest computed from edgeId and data.

The new creators are unable to update the royalty target and the fee route for their works.

The TitlesGraph contract is designed to be upgradeable, utilizing the UUPSUpgradeable pattern. However, it's instantiated via a constructor in the TitlesCore contract setup.

The FeeManager contract lacks an interface for the admin to grant or revoke roles.

Malicious users can block someone from acknowledging or deacknowledging an edge, affecting the sanctity of the data in the Graph.

The signature is malleable. When a signature is malleable, it means that it is possible to produce another valid signature for the same message (which also means the same digest).

- When the totalSupply of a market is 0, an attacker can take advantage of 1 and 2 to drain the entire protocol.

When a borrowed maturity is liquidated, canDiscount in Market::noTransferRepayAtMaturity() is false, which ignores the unassigned earnings.

The TARGETHEALTH calculation is correct, but returns the debt to pay considering that this debt corresponds to the average adjustFactor, which is false for more than 1 market, leading to significant deviation in the…

transferFrom() to receive the assets of the liquidator to pay the debt in Market::liquidate() is done only after performing the liquidation, making it impossible to liquidate users when seizeMarket == repayMarket.

At protocol launch, when previewFloatingAssetsAverage() is 0, a borrow may be taken with the lowest interest rate possibel due to the utilization being 0 when the average assets are 0.

The protocol does not prioritize repayment of debt in partial liquidations in the lowest Loan-To-Value tokens which leads to inefficient liquidations in which health factor because the average Risk-Adjust Factor is not…

Market::totalAssets() only accounts for the unassigned earnings of maturities that are in the future or during the past interval.

Setting new parameters in RewardsController::config() will lead to lost rewards if block.timestamp start and the rewardData.start was set in the future initially.

An attacker can update the variable floatingBackupBorrowed without updating the floating debt.

The RewardsController distributes rewards to both depositors and borrowers. When new rewards are available, the admin assigns them using the config function.

The earnings accumulator is not updated and converted to floatingAssets pre liquidation, leading to an instantaneous increase of balance of the liquidatee if it has shares which causes a profitable liquidation and the…

ZivoeYDL::earningsTrancheuse() calculates the target yield, senior and junior proportions based on daysBetweenDistributions.

OCLZVE::pushToLockerMulti() verifies that the allowances for both tokens is 0 after providing liquidity to UniswapV2 or Sushi routers, however there is a high likelihood that one allowance will not be 0, due to setting…

A bullet loan borrower can pay less interests by calling callLoan at the end of payment period. In OCCModular contract, the protocol can create loan offers, and users can accept them.

The ZivoeRewards.sol and the ZivoeRewardsVesting.sol contracts are a fork of the Synthetix rewards distribution contract, with slight modifications.

As emissionEnd and rewardRate can be freely set (although rewardRate only in the constructor), if emissionEnd x rewardRate is bigger than the amount of tokens sent to the staking contract, stakers may withdraw…

Rewards are emitted at a certain rewardRate, which means stakers that have been staking for longer receive more rewards. However, there is an edge case in which this does not happen.

getRemainingUnstakeTime() returns 0 if the user has not initiated unstaking. If staker.unstakeInitTime == 0 return unstakeTimeLock. Acknowledged

emissionStart is set in the constructor, but it has no use other than summing with the emissionDuration to set the emissionEnd.

Events in the constructor are not emitted. rewardPerToken update could emit an event. withdrawFees() is missing feesAccrued event. completeUnstake() is missing fee paid event. Emit events for all state changes.

Storage reads are expensive and should be avoided. This is usually done by caching storage variables in memory ones.

require() statements spend more gas than custom errors. Replace require() statements with custom errors. Acknowledged

completeUnstake() transfers tokens before deleting the information of the staker, not following the checks-effects-interactions pattern. Delete the staker mapping before transferring the tokens. Addressed in 67d85d0.

Constants should not be hardcoded due to readability purposes.

The codebase is using transferFrom() and transfer() which revert for weird ERC20 tokens. It should not happen as the MetaZero token implements the standard correctly.

If the position is a spread, it may be liquidated as one up to the minimum absolute value of the spot and perp positions.

state.cumulativeDepositsMultiplierX18 may go below 0 if an account is socialized or in the worst case, 0, which will have greater impact.

OffchainExchange::swapAmm() calculates the quote amount as -txn.amount.mul(txn.priceX18). Thus, if the price is negative, base and quote amount will have the same sign.

ClearingHouseLiq::liquidateSubaccountImpl() can finalize an account, repaying the bad debt from insurance or socializing the account, depending on the amount of insurance available.

When using Initializable.sol, it's a good practice calling disableInitializers() in the constructor, such that the implementation itself can't be initialized.

The signerBitmask is hardcoded to 7 when calling requireValidSignature(), meaning that nSigned would be incremented at most 3 times in checkQuorum() as 7 = 00000111.

Variables of type int128 are being upcasted to int256 and then downcasted again in the burnLp function in PerpEngineLp.sol and SpotEngineLP.sol, leading to silent overflow.

The isHealthy function in OffchainExchange.sol consistently returns true regardless of its parameter.

For example, in SpotEngineState::getInLpBalance(), the ratio is calculated first, likely incurring in precision loss as lpState.supply is expected to be bigger than 1e18 (used in .div()).

Endpoint::registerTransferableWallet() sets transferableWallets[wallet] to true, but a transferable argument can be provided. Delete the argument or replace true by transferable. Addressed in 38bbe76.

Risk parameters should be lower, higher or equal to 1e18, depending if short or long. For example, the long risk values should be smaller or equal to 1e18, or some of the functionality may get corrupted.

PerpEngineLp::burnLp() and SpotEngineLp::burnLp() have no slippage control parameters, which means that a user could get base and quote at a worse price/quantity than expected.

ClearingHouse::addEngine() initializes productEngine, which means it is uninitialized prior to the call.

In Endpoint::depositCollateralWithReferral(), the referral is set to referral code to default. It's also possible to do this by submitting a slow mode transaction of type BurnLpAndTransfer.

txType == TransactionType.ExecuteSlowMode allows the sequencer to execute slow mode transactions. However, these transactions can also be executed permissionlessly.

depositCollateral() and related functions transfer funds from the user, but only after the delay or the sequencer submission is the balance of the user updated.

PerpEngineLp::mintLp() assigns quote according to with the requirement that amountBase % sizeIncrement == 0.

Consider overriding OpenZeppelin's Ownable renounceOwnership() function if the project's roadmap doesn't plan to relinquish total ownership control. Acknowledged

ArbAirdrop::getClaimed() may revert after enough weeks have passed due to OOG. BaseEngine::getHealthContribution() may revert if enough productIds are set. Add initial and final weeks arguments. Acknowledged

Ownable2Step places some important checks, such as a 2 step ownership transfer procedure and should be preferred over Ownable. Replace Ownable2Step by Ownable whenever possible. Acknowledged

Some state changes are missing events: - ArbAidrop, all functions. Emit events for all relevant state changes. Acknowledged

The interface declares increaseAllowance and decreaseAllowance functions, which aren't part of the official ERC20 standard. Calls to such functions may revert with some tokens.

Using abi.encodePacked() with dynamic types, as seen in Verifier.sol on L160 and in Endpoint.sol on L749, isn't advisable when feeding the outcome into a hashing function like keccak256(), due to hash collisions.

RiskHelper::isoGroup() is called in various functions across the code such as ClearinghouseLiq::finalizeSubaccount(), and ClearinghouseLiqsettlePositivePerpPnl().

Rewards allocated to compounding shares are only swapped to tokens 0 and 1 if the sellPortion exceeds the rewardSwapThreshold.

PHyperLPoolSwapInside::rebalance() checks price manipulation before swapping to prevent sandwich attacks and after swapping as a way to mitigate slippage.

keepPortion and sellPortion are based on the total number of shares of each type. However, compoundShareRate is expected to increase due to accumulating rewards.

Performed fuzz tests of the InRatioSwap.getInRatioSwap() function showing that the error is always smaller than 1e10.

PCLBaseSwapInside() is missing a disableInitializers() call in the constructor, allowing the implementation contract to be initialized.

Rebalance changes the lower and upper ticks in PHyperLPoolSwapInside and withdraw from the farming contract if it is allocated.

PHyperLPoolSwapInside::swapWithCustomData() uses unsafe .approve() which does not return a bool on some tokens, reverting. Use .forceApprove(). Addressed in 0c8de23.

PHyperLPoolSwapInside::swapWithCustomData() checks price manipulation for pool; however, the router chosen may route through a different pool, rendering the price manipulation useless and leading to MEV.

PHyperLPoolSwapInside::swapWithCustomData() performs swap using a router provided as argument in swapData.

In PHyperLPoolSwapInside::depositNftToFarm(), the NFT transfer will succeed if the farmingContract is incorrectly set.

PHyperLPoolSwapInside::swapWithCustomData() does abi.decode of the returnData, which is already a bytes variable. revert(string(returnData)); should be enough Addressed in 0c8de23.

Users can transfer tokens to the StakingAssetManager in return for a note commitment of the corresponding zkToken.

All functions that require a merkle root to verify note inclusion must validate that the merkle root is valid. Otherwise, attackers can forge a merkle root that includes a fake note and steal all assets.

Here is a writeup about the pattern. Essentially state changes should be handled before interacting with external contracts (for example, when sending ETH via .call(""), to avoid reentrancy.

CurveMultiExchangeAssetManager::curveMultiExchange() receives arguments assetIn, assetOut and route. Route is defined as Array of [initial token, pool or zap, token, pool or zap, token, ...] .

DarkPoolAssetManager::join(), DarkPoolAssetManager::joinSplit() and DarkPoolAssetManager::swap() don't check if the 2 notes are the same. Thus, using only 1 note, it's possible to double the amount of funds available.

The curve exchanges allow users who deposited to use their note commitments to swap for other assets.

The nullifier is formed from rho and the schnorr pubKey. This means that depositing with different assets and amounts will still have the same nullifier, leading to the inability to move the following deposits with the…

In curveRemoveLiquidity(), the note footer should not be allowed to be 0 if assetOut is non null, as this could lead to duplicate note footers. Revert if the note footer is 0 but the corresponding asset out is not.

UniswapLiquidityAssetManager::validateCollectFeesArgs() allows using the same note footers as it checks for nullifiers, when it should check for note footers. Check for note footers correctly. Addressed in 8b300f0.

CurveAssetManagerHelper::validateAssets() allows sending assets with indexes bigger than the maximum allowed of a pool (numcoins). This could lead to users losing tokens or unexpected behaviours.

An example of this issue is for example here. If the last bit of isLegacy is 0, it checks address(this).balance, otherwise it IERC20(WETHADDRESS).balanceOf(address(this)).

CurveMultiExchangeAssetManager::exchange() allows sending any route, which is of type Array of [initial token, pool or zap, token, pool or zap, token, ...].

DarkpoolAssetManager::depositETH() and DarkpoolAssetManager::depositERC20() allow choosing any owner, while the msg.sender still receives the deposit.

CurveLiquidityAssetManager::curveAddLiquidity() always spends the nullifiers, even if the amounts used are 0.

The slippage is computed as: This gets the mintAmount value post price manipulation, rendering the slippage protection useless. Also, hardcoding a parameter of 95% slippage is not ideal. Here is a similar finding.

The relayer fee used throughout the codebase is not enforced anywhere, such that it's possible to frontrun a legit transaction and set a much lower/higher relayer fee, harming the relayer/user.

When using DarkpoolAssetManager::Split(), there is no check to make sure that the 2 resulting notes are not equal, making it impossible to use the second note after the first one as the nullifier will be used by then.

Service fees are fixed, but they should depend on the asset, as their price differs significantly. Users are likely to arbitrage this. Set a mapping for service fees. Acknowledged

If isETH, but args.isLegacy is 1, it wraps to weth and allows pool, see here. if args.islegacy is 0b10 and the asset is ETH, it does not send ETH to the pool, here.

All Uniswap interactions are missing a deadline argument and are not setting the following slippage protection arguments in UniswapLiquidityAssetManager: - amount0Min and amount1Min in MintParams.

Fee on transfer tokens are not correctly dealt, as the transferFrom() call is expected to transfer exactly the requested amount. However, this is not the case for tokens that charge a fee on transfer.

ETH transfers should revert if they fail. Although this issue alone will not lead to exploits, it increases the attack surface. Revert if the transfers fails in: - Curve remove liquidity. - Curve multi exchange.

ZKTokens are minted 1:1 to the underlying assets, so should inherit the same decimals. For example, if USDT is the underlying asset, and 100 USDT are locked, it will mint 100e6 ZKTokens.

Tokens are locked and unlocked, minting and burning zkTokens in the process, respectively.

Curve pools vary significantly between one another, which could lead to unexpected behavior.

VerifierHub::setVerifier() should emit an event when a verifier is set. Emit events for all relevant state changes. Addressed in 8b300f0.

Setting note commitments, nullifiers and note footers should never happen after they are already set, in which case the transaction should revert, as it is some clear attempt of an exploit.

ERC721 implements a callback when transferring tokens. Some tokens, such as ERC777 implement callbacks when transferring. Both these tokens could lead to reentrancy.

1) the hashes are not unique, num A and num B = A + P will yield the same % 2) doing % P introduces a bias in the hash where smaller values are more frequent.

Uniswap positions may collect fees in only of the tokens, leading to 0 fees in the other. This will cause the fee manager to revert in calculateFee(). Skip transferring the funds if the amount is 0. Addressed in 8b300f0.

MerkleTreeOperator::getMerklePath() searches the tree in linear time for the index of the requested noteCommitment. Thus, it will run out of gas or timeout when using rpc providers when enough leaves are added.

stakingAssetManager in ZKToken can be made immutable as it is set in the constructor and never changed. Set stakingAssetManager to immutable to save gas on reads. Addressed in bff8628.

In StakingOperator::setUnlockWindow(), unlockWindowStart and unlockWindowDuration are uint256, so they can not be negative. However, the code checks for negative values, which is a waste of gas.

The constructor of StakingOperator is As can be seen, it receives a flag isUnlockWindowActive, but never states the state variable isUnlockWindowActive. This may be intended, but flagging it because it is not clear.

In StakingAssetManager::unlock(), fees are forwarded before marking the nullifier as used by calling postWithdraw().

Some variables such as these are initialized to 0, which is not necessary. Consider not initializing the variables as it is not required. Addressed in 8b300f0.

UniswapLiquidityAssetManager::uniswapLiquidityProvision() could return the tokenId for better verbosity. Return the tokenId. Addressed in 8b300f0.

Some spelling errors can be found in the codebase: Fix the spelling mistakes. Addressed in 8b300f0.

UniswapRemoveLiquidityInputs struct in UniswapInputBuilder has a parameter named positionNoteCommitment which is not required. Remove the mentioned parameter. Addressed in 8b300f0.

There is no identifier in the circuits corresponding to the method that the proof relates to. This means that if 2 circuits have exactly the same inputs, the same proof could be used for more than 1 method.

The note footers are registered twice. Only register once. Addressed in 8b300f0.

0 address checks in the constructor are a safety check to ensure addresses are correctly set. The BaseAssetManager, which is inherited by all pool managers, could perform these checks.

OstiumTradingStorage::firstEmptyTradeIndex() and OstiumTradingStorage::firstEmptyOpenLimitIndex() return index 0 if they can not find an available index.

Traders who continually update their stop losses before the SL timeout expires will never face liquidation.

Ostium uses a velocity funding rate update, so the actual change is the integral of the velocity of the past time.

Presently, when there isn't adequate liquidity in the OstiumVault to cover profits for trades with substantial positive PnL, a revert occurs.

The execution of a limit order within the OstiumTrading::executeAutomationOrder() function lacks a check to determine if the contract is paused. Addressed in fa95bb0.

When opening trades it is checked if the collateral is within limits for each pair in OstiumTradingCallbacks::withinExposureLimits(). The same should be done in OstiumTrading::topUpCollateral().

OstiumTrading::closeTradeMarket() and OstiumTrading::topUpCollateral() can be used to frontrun liquidation calls whose trigger has already been set, making the liquidation fail in OstiumPriceUpKeep::performUpkeep(),…

At the moment oracle fees are payed after the trades have been finalized, either in registerTrade(), openTradeMarketCallback() or closeTradeMarketCallback().

The calculation of the maximum deposited collateral per group, erroneously divides by 100, assuming it refers to 100%. However, the maxCollateralP variable, specified here, has a precision of 2 decimals.

OstiumPairsStorage::updatePair() updates a pair's attribute but does not check the new values. Add the pairOk() modifier to updatePair(). Addressed in 7fe937e.

OstiumPriceUpKeep::performUpKeep() should unregister pending market orders or disable triggers and pending limit orders if the execution fails. However, it is not dealing with all the revert scenarios, for example: 1.

In OstiumTrading::updateSl(), a validation is present for the maximum stop loss (SL) distance. However, there is no validation for the opposite scenario, as seen in OstiumTrading::openTrade() at L202.

USDC incorporates a blacklist functionality that reverts any attempt to transfer funds to a blacklisted address, even if the transfer amount is 0.

Some instances, namely in OstiumPairInfos::getPendingAccFundingFees(), cast int256 to uint256. Avoid casting directly and use a wrapper library instead such as SafeCastUpgradeable. Addressed in 38b09ab.

getPendingAccFundingFees() computes the number of blocks to the limit by subtracting absLastFundingRate to maxFundingFeePerBlock. setMaxFundingFeePerBlock() allows setting the max to any value below MAXFUNDINGFEE.

In OstiumTradesUpKeep::getOpenOrdersToTrigger(), orders are grouped together for execution without some important checks.

Within the Delegatable::setDelegate() function, there exists a check to determine if a contract is executing the function, restricting the action solely to Externally Owned Accounts (EOAs).

The condition executed when the OpenOrderType is of type STOP, as seen here, triggers when the current price is greater than or equal to the target price.

utilizationThresholdP should never be set to the max 10000 or it will lead to division by 0 when calling OstiumPairInfos::getUtilizationOpeningFee().

OstiumVault::lockDiscount() reverts if maxDiscountThresholdP == uint16(100) PRECISION2, but the constructor and updateMaxDiscountThresholdP() allow setting it to this value.

reqIDpendingAutomationOrder store the trader, pairIndex and index.

Limit orders usually trade at the specified price; however, OstiumTradingCallbacks::executeAutomationOpenOrderCallback() registers the limit order t.openPrice at priceAfterImpact, which would be different than the…

OstiumPairsStorage::getAllPairsMaxLeverage() reverts if enough pairs are created due to OOG.

Ownable uses a dangerous pattern of setting the address without proper checks. This means that if a mistake is made and ownership is transferred to the wrong address, the owner functionalities would be forever lost.

OstiumRegistry allows renouncing ownership by inheriting Ownable, which could be catastrophic if triggered by mistake. Consider overriding renounceOwnership() and revert if called to disable this functionality.

expected to be equal, as seen in OstiumLinkUpKeep:getUnderfundedUpkeeps(). Either do: or use an array of structs containing the 3 parameters. Addressed in ae32282.

If the number of registries in the sregistries set grows too large, it will revert when calling getWatchList() due to OOG. Add a paginated function to get the watch list. Acknowledged

OstiumTradingCallbacks::executeAutomationOpenOrderCallback() fetches the openLimitOrder before checking for its existence, which will end up reverting if it does not exist, not finishing execution.

OstiumTrading::openTrade() correctly checks isPaused to not allow opening positions when the protocol is paused.

In OstiumLinkUpKeep, functions setWatchList(), addToWatchList() and removeFromWatchList() don't handle 0 length array inputs, which could lead to incorrect state. Explicitly check for empty arrays. Addressed in c9ebe98.

When withdrawing ETH in OstiumPriceUpKeep::withdrawEth() using the deprecated transfer() function will make the transaction revert when: 1. The claimer smart contract does not implement a payable function. 2.

When using Initializable.sol, it's a good practice calling disableInitializers() in the constructor, such that the implementation itself can't be initialized. Use: Addressed in 1a1c899.

If additional variables are added to the Delegatable contract, conflicts may arise with the storage slots previously written on the proxy. To mitigate this issue, insert a storage gap in the contract: Acknowledged

The leveragedPos variable in OstiumTrading::executeAutomationOrder() is calculated on L443 and L465, but its value isn't utilized anywhere in the function. Addressed in 27e6f2c.

The usage of require statements instead of custom errors is evident in both OstiumLinkUpKeep and particularly in OstiumVault.

Throughout the codebase, variables representing percentages maintain a precision of 2 decimal places.

Unnecessary typecasting of msg.sender to address is seen on L34 in OstiumLinkUpKeep::onlyGov() and on L32 in OstiumRegistry::onlyGov(). This redundancy is unnecessary as msg.sender already returns an address.

Several instances of magic numbers are present throughout the contracts: - The validation for slippageP in OstiumTrading::openTrade() on L169. - L117 in OstiumOpenPnl::getOpenPnl().

Unused Imports import '@openzeppelin/contracts/utils/Address.sol'; '@chainlink/contracts/src/v0.8/automation/interfaces/ILogAutomation.sol'; // duplicate import './interfaces/IOstiumOpenPnl.sol';

When opening a LIMIT or STOP order, the first empty index is calculated using OstiumTradingStorage::firstEmptyOpenLimitIndex() on L209.

At L284 in OstiumTrading::closeTradeMarket(), the trade info can be reused from the previous call to OstiumTradingStorage::getOpenTradeInfo() at L265 to conserve gas by avoiding redundant function calls.

The variable sconfig is not set in the initializer function, which is preferable.

The Chainlink documentation states that verifying data streams via native blockchain gas tokens and their ERC20-wrapped versions incurs a surcharge when compared to LINK payments.

In OstiumTrading::updateOpenLimitOrder(), unnecessary fields of the OpenLimitOrder struct are updated.

Orders in OstiumTrading::executeAutomationOrder() will not execute if they are in the timeout period.

Enum OrderType in IOstiumPriceUpKeep has a UPDATESL property which is never used. Remove this property if it is not used. Addressed in ab4e1ae.

In OstiumTradesUpKeep::getLimitOrdersToTrigger() and OstiumTradesUpKeep::getOpenOrdersToTrigger(), tradesToTrigger are returned with 1 extra length due to incorrect ++tradesToTrigger operation.

OstiumPriceUpKeep::performUpkeep() decodes the verifierResponse setting an expiresAt as uint192, when it is in fact a uint32 in both Basic and Premium reports.

OstiumPairsStorage::updateGroupCollateral() does not check for the pair's index existence, which means that it would update group with index 0 incorrectly.

Occurences OstiumLinkUpkeep, WrongParams. Acknowledged

Delegatable decodes the revert reason of a revert in a way that may fail. It adds 4 bytes to the result pointer, which may mess up the length of the result variable, see more details here. Don't decode the reason.

OstiumOpenPnl::average() calculates the average of all the nextEpochValues, which is an expensive operation (depending on requestsCount and could be replaced by a state variable tracking the cummulative sum. Acknowledged

OstiumOpenPnl::nextEpochValuesRequestCount can be removed as it is the same as checking nextEpochValues.length. Acknowledged

The second decoding on L126 of the data parameter is identical to the first and does not assign its result to any variable. This appears to be redundant and serves no discernible purpose.

safeApprove should only be called when setting an initial allowance, because it reverts when a non-zero approval is changed to a non-zero approval.

OstiumLinkUpKeep:removeFromWatchList() currently iterates over the full sregistryWatchLists to find the keeperIds and delete them if equal. This has O(n^2) complexity, which could cost a significant amount of gas.

OstiumLinkUpKeep has typos in errors WhatchlistEmpty , HasWhatchlist and NoWhatchlist. Addressed in 119db4a.

AmmRouter:removeLiquidity() subtracts the long notional by the short notional if the size is bigger than 0 (contrary if size is smaller than 0), which may revert if the short notional is bigger than the long one.

For very low notional amounts, when adding liquidity, invariantIncrease is always bigger than 0 (not true for the first depositer though), but the corresponding quote and base amounts might be 0.

The price deviation check will look at the last price snapshots; however, it does not actually enforce that these are from past blocks.

Functions addLiquidity() and removeLiquidity() change mark price but don't call updateMarkPrice().

PositionManager:match() looks for all the orders for their trigger values as some order may have been filled/deleted.

AmmRouter:shitPool() increases the index if the price is within 100 of the upper bound of the current pool price.

PositionManager:reversePosition() reduces the notional by the amount required to close the previous position, fetching the value using the getters before.

staticFillToAmm() checks twice the size to fill. Remove the second check as it is never triggered. Addressed in 1c107ae

The abstract contracts use gaps to change storage later if required, but AMMRouterBase is missing the private keyword. Addressed in 1b573fa

The following places do an unchecked increment/decrement, but the index could underflow and become type(uint256).max. This has no consequences with the code as is, but it's better to revert for underflows. Acknowledged

AmmRouter:liquidateMaker() calls unregisterPoolsFromMaker() with all the pools, which means that it could simply delete the pools from storage. Addressed in 0ea5723

ammRouter:trimDuplicatePools() eliminates duplicate pools in the input by comparing every element of the array for duplicates, which is very expensive.

AmmRouter:addLiquidity() does not have an explicit check regarding the existence of the pool with the given id, making it only revert when calling poolsAdded[i].addLiquidity() due to internal EVM checks.

AmmRouter:setPools() is an admin function but could still benefit from input validaiton to prevent mistakes.

Function closePosition() of the ClearingHouse contract checks if the size sent as an argument to this function is smaller than the size of the position to close.

distribute() uses the argument amountOutMinimum as slippage control, but the function is permissionless, so malicious users can trigger swaps with 0 minimum amount out. Set up a keeper role to distribute rewards.

reclaimToken() does not use SafeERC20.safeTransfer(), which could lead to stuck tokens. Acknowledged.

Swapping with block.timestamp as deadline means validators may do MEV up to the minimumAmountOut. Addressed by disabling taxes.

Tax sells specify a minimumAmountOut of 0, which is vulnerable to MEV. Keep in mind that this is usually a design choice of tax tokens to avoid other issues. Addressed by disabling taxes.

createAmmPairWith() in initialize() reverts if the pair exists, which means that the contract could be DoSed. Check if the pair already exists and skip creating if it does.

Rewards are emitted at a rate of rewardTokenPerBlock. However, when distributing, the received reward tokens are added directly to rewardPerToken, instead of being left in the contract and letting rewardTokenPerBlock…

Increasing shares infinitely allows an attacker to steal all rewards. It's possible by doing: 1.

A pair with zero address may be added to the pairs mapping which means burns will be taxed. Revert if the pair does not exist in recordAmmPairWith(). Addressed by disabling taxes.

totalShares update subtracts before summing, which could lead to underflow in edge cases (when something unexpected happens). Acknowledged.

During the dead block period, before trading has started, users may send tokens to an address to maliciously blacklist it.

Paying fees to the marketing operator uses .transfer(), which only forwards 2300 gas, which could lead to reverts if the operator has any logic in the fallback or receive functions.

Occurrences Acknowledged.

Openzeppelin Ownable2Step changes transferOwnership() to a 2 step procedure, which means that to change an owner, first a pending owner is set and only then this account must accept ownership.

Occurrences Acknowledged.

send() does not check the owner of the to be sent nfts, which allows anyone to send nfts to itself on behalf of someone else. See the POC here. Add isApprovedOrOwner() on the nfts to be sent in send().

deTokenize() allows users to burn their nfts with a certain string url. The function is missing access control, such that anyone can burn any nft, POC here.

send() allows users to send messages with 0 gas limit if the gasLimit mapping has not be seen for a certain peer. This will likely make the transaction revert on the destination chain, losing the funds. POC here.

send() sends the tokens to the same address that called the function. Some smart contract wallets may have different addresses on different chains (or even chains with different address generation).

mint() in mintGenesis() may send tokens to an address that can't deal with ERC721. safeMint() would be recommended; however, it would introduce additional issues, namely reentrancy and possible malicious reverts.

Function mint() and lzReceive() mint a nft to a certain address, which may lead to lost tokens if safeMint() is not used instead.

Only 999 tokens can be limited according to the code, but the comments indicated that 1000 tokens should be minted on genesis. See the POC for confirmation. Change genesisCounter to 0. Addressed in cd1bb46.

State changing variables should emit events for data fetching purposes. The following variable changes are missing events: Addressed in a7dab03.

Calling renounceOwnership() would block the owner from very important functions, which would be problematic.

Ownable performs a simple ownership transfer by setting the owner to a new address. This is dangerous and could result in an invalid owner being set.

The genesisMint() function could cache genesisCounter while looping: Addressed in 6ff3111.

setDefaultRoyalty() in the constructor is overriding the BasicRoyalties constructor to 500, regardless of the parameter sent. Addressed in 28850d5.

lzReceive() only uses the payload parameter, so the other names can be removed, leaving only the types: function lzReceive( Origin calldata, // struct containing info about the message sender address, // the Executor…

The LzEndpoint, a minter, the royalty fee and the pay in zero flags [([1]]( [[2])]( are hardcoded values, but should be placed as constants instead. Acknowledged

Constant variables are cheaper as no storage writes are required, so genesisLimit should define the constant keyword. Addressed in a79fbc8.

Solidity versions above 0.8.0 enforce pragma abicoder v2; by default. Addressed in 03b38ab.

The function PowerToken.buy is used to purchase tokens in auction. The price of those tokens decreases with time, following a dutch auction model.

When calling MinterGateway.updateCollateral, an array of timestamps gets passed. That is because the digest signed by a validator includes an updating timestamp.

Because there's an unchecked total earning principal addition in MToken.addEarningAmount, the MToken contract would be subject to a potential silent overflow of principalOfTotalEarningSupply.

When calling MinterGateway.updateCollateral, an array of timestamps gets passed. That is because the digest signed by a validator includes an updating timestamp.

In the StableEarnerRateModel.getSafeEarnerRate function, there's a log calculation detailed in a comment: The code implementation of the log argument calculation follows the comment, but using fixed point arithmetic…

The MinterGateway.updateCollateral function should be called once every updateCollateralInverval. If the minter fails to do this a penalty will be imposed on him next time he tries to update his collateral.

The DistributionVault.getClaimable function rounds down during the division operation used to calculate claimable, resulting in dust amounts being locked in DistributionVault.

The function StandardGovernor.propose is used to create a new proposal to be executed in the StandardGovernor.

In function MToken.mint, when the account is an earner, the principal gets added to both its raw balance and to the global variable principalOfTotalEarningSupply.

The function EpochBasedInflationaryVoteToken.getUnrealizedInflation calculates a given account's inflation which has not been added to their balance yet.

In the MToken.mint function, there's a check to make sure the total principal doesn't become larger than a uint112: if ( principalOfTotalEarningSupply + getPrincipalAmountRoundedDown(totalNonEarningSupply) =…

In the BatchGovernor contract, a new proposal will generate a proposal id by using the ZeroGovernor).

In the MinterGateway.cancelMint function, an approved validator cancels a mint proposal by passing the minter address and mintId. The function will revert if the the minter's proposal doesn't correspond to the input id.

The StandardGovernor contract inherits from IGovernor, which states the following: But the StandardGovernor contract implements quorum functions in the following way: The documentation from Tally states that it "needs…

In the MinterGateway contract, the updateCollateral function can set a collateral value to any value, provided the value is no larger than type(uint240).max and the data is signed by enough validators.

Most contracts are not implementing their interface methods with override keywords.

There are some contracts that don't fully implement the interface with the same name. For example, IERC5805 defines functions such as delegates, getPastVotes and getVotes.

In MinterGateway.deactivateMinter the minter's entry in minterStates is deleted using the This action sets all types of that specific entry to its default value, and so, there is no need to set isActive to false as this…

In MToken.startEarning function, the balances mapping is stored in the mBalance variable. MBalance storage mBalance = balances[account];

Since StandardGovernor.votingPeriod function will always returns 0, voteStart will always be equal to voteEnd.

There's several places in the protocol where proper code validation is missing by, instead, relying on panic errors.

In MToken.startEarning the StartedEarning event will be emitted even if the calling account is already earning: The same happens in the MToken.stopEarning.

The following comment is present in StandardGovernor's execute function: // Proposals have voteStart=N and voteEnd=N, and can be executed only during epochs N+1 and N+2.

The currentEpoch value is derived from BatchGovernor.clock, which can never be zero. But the execute functions of both StandardGovernor and ThresholdGovernor check if currentEpoch is zero.

The PowerToken.divideUp function checks if a result of a multiplication "wrapped" around the maximum number, i.e.

The function ThresholdGovernor.execute executes a successful proposal.

The PureEpochs library implements a variety of functions centered on the definition of epochs.

The ERC5805.checkAndIncrementNonce function makes sure the nonce being used in a signature is the right one. If it is, it increments it: nonces[account] = currentNonce + 1; // Nonce realistically cannot overflow.

The BatchGovernor contract supports castVoteWithReason to be compatible with certain governors.

The ERC20Extended.transferFrom function is using approvefor changing allowance, and this internal function always fires up an Approval event.

Function ERC712.revertIfError goes through the different error possibilities in SignatureChecker.Error and raises the right error accordingly. But some are missing: InvalidSignatureS and InvalidSignatureV.

The IERC3009 defines a token interface following the EIP3009 standard.

On a limit order update, in updateLimitOrder(), changing amm is not being taken into account.

On the merge of a liquidator position following a liquidation, the realizedPnl from the previous position is not added to the marginToRemove() variable, but it is removed from the openNotional, leading to loss of yield…

search() calculates the final tick as if the order is completely filled on the amm.

setFundingPeriod() calls settleFunding() in the ClearingHouse. It may be the case that someone frontruns setFundingPeriod() with a call to settleFunding(), making the setFundingPeriod() transaction revert.

When filling limit orders, if the trader doing the openPosition() call is the same as the maker of the limit order, it will delete the limit order.

getPositionNotional() fills positions to the amm if it can't find limit orders.

There is no limit in the amount of triggerOrders, which means that the array may grow too large and make transactions revert due to the gas usage exceeding the block gas limit when calling getTriggerOrders().

removeLiquidity() updates the position quote and base amount based on the removed quote and base amounts. However, if the proportions changed since the position was opened, it may underflow.

removeLiquidity() removes the margin from the position pro-rata to the removed shares, without considering the previous funding payment.

When changing contract owner, a two step process is recommended. Using this method, first a pending owner is set, which then has to accept the role in order to become the contract owner.

The totalPositionSize mapping is used to hold the position information for an amm using the TotalPositionSize struct, which has the following fields: - int256 netPositionSize - uint256 positionSizeLong - uint256…

Some state changes are not emitting events - - Addressed in multiple commits

For example, getBaseValue() calculates the base amount according to the k using the virtual reserves;

When changing contract owner, the keeper permission of the previous owner is revoked, and this permission is given to the new owner.

In solidity, structs can often be packed into fewer words to save gas on storage loads/stores. In the code, the following structs can be optimized in this way: LimitOrder, TriggerOrder, TwapInputAsset, MarketOrder.

To avoid differences between the function declarations in the contracts and their interfaces, it is a good practice for contracts to inherit their interfaces.

In the positionManager library, internal functions to increase, decrease, close or even reverse a position tend to not revert and instead rely on a response system to inform the parent function if the call was…

Using assembly blocks in solidity isn't recommended, unless it presents as obvious advantage, usually an optimization, that can't be performed with standard solidity.

While during testing it's possible to get the relevant information using other methods (console.log comes to mind), when the contract is deployed and transactions are live, it's harder to debug without the information…

Usually mul() and div() do multiplication and division, respectively, without scaling by 1e18 (commonly used by SafeMath). However, NFTPMath mul() and div() scale the operations by 1e18, misleading code readers.

Users can avoid being liquidated if they frontrun liquidation calls with a liquidate call with 1 wei.

repay(), liquidate() and liquidateWLp() transactions revert if users approve the exact repay amount they need in the frontend and only after some blocks have passed is the transaction settled.

The impact of this finding is more on the marketing/data fetching side, on exchanges it would appear that the shares are worth less VIRTUALSHARES than the underlying token.

When users deposit wLP tokens as collateral, then they are checked to be whitelisted. Later, it's possible that for some reason wLP token will be blacklisted by governor.

In the Api3OracleReader.sol, the code assumes tha the timestamp returned from oracle is always in the past. // e.g.

The queue withdrawal manager limits the sharesToProcess in processRedemptions() to the totalShares. This might make the call revert if non malicious users remove shares or do manual redeems.

Loops revert due to OOG errors if too many iterations are performed. Some functions will stop working if these loops grow too large.

In the MapleLoanV502Migrator, the only check is that the factory address is a valid instance. Thus, it's possible for a loan with the old factory to be updated and use the same old factory.

In the queue-based version of the withdrawal manager, lenders don't control when they leave the protocol, so they don't choose when the conversion from shares to asset is performed, which may lead lenders to receive…

Queue MapleWithdrawalManager: In processRequest(), queue.requests[requestId].shares -= processedShares; can be replaced by queue.requests[requestId].shares = request.shares processedShares;

- MapleWithdrawalManager:setExitConfig() could be performed by the operational admin, since it can call setPendingDelegate() and acquire the permissions anyway.

- The owner of redeem(), withdraw(), removeShares(), requestRedeem() and requestWithdraw() must be allowed and transfer(), transferFrom() check the sender of the shares now.

On the Batch.sol contract, anyone can call withdrawFulfill() with the current cycle id, incrementing the cycleInfo[currentCycleID].withdrawRequestsFullFilled and setting the…

Exchange:swap() sets minAmountOut to 0 and in the plugins the deadline is set to block.timestamp.

BatchOut:executeBatchWithdrawFromStrategyWithSwap() calls strategyRouter:withdrawFromStrategies() with the token having the most requests shares.

Anyone can schedule as many withdrawals as they want in BatchOut:scheduleWithdraw(), specifying little shares to different withdrawTo addresses.

A token might be supported at time A, letting users withdraw in BatchOut using this token.

BatchOut:scheduleWithdraw() allows sending a withdrawTo argument of 0. Some tokens, such as USDT, revert when transferring tokens to address 0.

StrategyRouterLib:rebalanceStrategies() withdraws excess funds from strategies and adds it up to the router balance.

Currently the price is calculated as uint256 routePrice = FullMath.mulDiv(uint256(sqrtPriceX96) uint256(sqrtPriceX96), precision0, 2 (96 2));.

StrategyRouter:withdrawFromStrategies() withdraws from the idle strategy of the withdraw token and then tries to withdraw from idle strategies and later strategies with other tokens.

Some tokens may revert on 0 amount transfers. This means that if someone schedules a withdrawal of 1 share, it could convert to a 0 amount and halt all the other withdrawals.

Anyone who wants to withdraw from a batch via Batch:withdraw() can be frontrunned by an allocateToStrategies() call, which increases the cycle id, making the withdrawal revert.

Strategies are allocated funds pro-rata to their weights. However, when the target of a strategy is reached, it won't be allocated any more funds.

Lines 665 and 666 of the StrategyRouterLib have the following code: currentTokenDatas[j].currentBalance -= desiredAllocationUniform; currentTokenDatas[j].currentBalanceUniform -= desiredAllocationUniform;

Some tokens have a fee on transfer, such as USDT, although it is disabled currently (line 127 and 177).

In Batch.sol, function withdraw() burns the receiptId after transferring the tokens to the user.

The msg.value sent to Batch:deposit() might be bigger than the depositFeeAmount, which means that the contract will receive more native than the fee.

In Batch.sol, function getDepositFeeInBNB() calculates the fee amount in BNB, considering that the stablecoin price is 1 USD, which may not be true, as we've seen with USDC in the past.

Delegate mechanism in VotingEscrow allows infinite votes in voteforgaugeweights() in the GaugeController.

The MarketFactoryfund calls the specified market's MarketclaimFee function. This will send the protocolFee to the MarketFactory contract.

Market advances in the id of the Global and Local states by fetching latestVersion and currentTimestamp from the oracle, increasing it if there is an update.

Malicious users could abuse the accounting error to immediately start getting rewards belonging to others after staking, leading to a loss of reward tokens.

Since claimRewards accounts for rewards with balanceBefore/After, and anyone can claim Convex rewards, then attacker can DOS the rewards and make them stuck in the LiquidationRow contract.

This result in rewards not being recorded by LMPVault and ultimately frozen in the contract.

LiquidationRow acts as an orchestrator of claiming process. It liquidates tokens across vaults using the liquidateVaultsForToken function. This function has a flaw and will revert.

The rewards are destributed in a MasterChef style, which takes snapshots of the total accrued rewards over time and whenever someone wants to get the rewards, it subtracts the snapshot of the user from the most updated,…

The previewRedeem and redeem functions deviate from the ERC4626 specification.

Malicious users could lock in the NAV/Share of the destination vaults, resulting in a loss of fees. The collectFees function only collects fees whenever the NAV/Share exceeds the last NAV/Share.

The RewardsController from AaveV3 rewards are separate from the yield accrued and can be several different tokens. Thus, they should be handled differently.

Currently totalReserves is updated based on deposits and withdrawal amounts to the reservePool. This means that rewards accrued in the strategies will not be accounted for, effectively not earning any yield.

Increasing the network total is done via the increaseNetworkTotal() function call. Given that glAVAX allows instant withdrawals, it's very easy for MEV bots to steal rewards from legitimate users.

The balance available for withdrawals is tracked in address(this).balance. The implementation itself is correct and users will have enough liquidity to withdraw given enough balance is accumulated.

In the function rebalanceWithdraw() if it is necessary to withdraw from WAVAX in order to satisfy a withdrawal, the code will compare the balance of the WAVAX contract and how much is needed to satisfy the withdrawal.

When depositing or withdrawing in the ReservePool, it deposits/withdraws individually from the strategies based on the weights. In the case of withdrawals, the transaction might revert.

The current withdraw snapshot implementation allows users to withdraw a certain amount after other users withdraw this amount.

If a strategy stops working it will become impossible to use the Reserve Pool. The functions deposit(), withdraw() and withdrawAll() will no longer work.

The functions withdraw() and claim() make transfers directly to the user using payable(user).transfer(amount). .transfer can only forward 2300 gas which means it can fail for some contracts and stop withdraws.

When changing important addresses, it's best to use additional safety measures to prevent wrong addresses from being set.

The ReservePool always deposits and withdraws according to the specified strategy weights. As the strategies have different yield rates, the actual weights will change over time.

rebalance() in glAVAX deposits to the ReservePool if there is available liquidity (balance) and the currentReserves are smaller than the reserveTarget.

The current implementation of the receive() function in glAVAX allows any smart contract to call it, leading to lost funds. Refactor the function to only allow transfers from wAVAX. Currently being reviewed by the team.

Under normal circumstances the usage of transferFrom() in the function withdraw() in AaveV3Strategy and in GReservePool would revert as it would be necessary for the contract to approve itself.

The strategies are stored in the ReservePool as a mapping strategies and length strategyCount. This could be reduced to an array of Strategy, Strategy[] public strategies;, increasing readability and gas savings.

Sometimes the quantity of shares on hand is available instead of the corresponding wAVAX amount.

In function rebalance(), the withdrawAmount from the ReservePool can be computed inside the first if statement. Change the code to Currently being reviewed by the team.

The checks-effects-interactions pattern should be used whenever possible, even if apparently it has no consequences. There are some instances specified in the relevant links where it isn't followed.

Inverting if statements can help increase readability and decrease overall code complexity. Take a look at the following example.

In function withdraw(), checking if (avaxAmount 0) is not required since it is Remove avaxAmount 0 from the if statement. Currently being reviewed by the team.

Global and user withdrawal requests are stored in a mapping and a variable tracking the length respectively.

In the function withdraw() the function borrowLiquidity() is called to borrow funds from the lending pool if necessary.

The code follows the convention that storage variables start with an . In glAVAX some variables have the others do not. They should all follow the same rules to increase code readability.

Storage reads should be avoided whenever possible to save gas, which can be achieved by caching the variables in memory.

The balance of the user when withdrawing against the amount passed in as argument is already checked in the withdraw() function, there's no need to check again in withdrawRequest().

Typo in fufillWithdrawal(). Currently being reviewed by the team.

In the function withdraw(), the variable amountTransfered is going to be equal to the biggest of two variables, either totalWithdraw or amount.

There are two constants in this contract (maxStrategies and defaultStrategyWeigth), they should use the naming convention of being in capital letter the same way it is done in the other contracts.

In the function repay() the NatSpec comment says Anyone can repay a loan for anyone. This was true in the older version but not anymore, therefore this part of the comment should be removed.

In GLendingPool the events Borrowed and Repaid can have the parameter user as indexed. Currently being reviewed by the team.

If an import is never used it should be removed to save on code size. In wglAVAX imports PausableUpgradeable, IWAVAX, IGReservePool, IGLendingPool, AcessControlManager and GlacierAddressBook are never used.

The NatSpect comments of most of the functions in the contracts only have an explanation to what the function does. It is good practice to also add an explanation of the inputs and output of the functions.

The TOFT available in the TapiocaOFT contract can be stolen when calling removeCollateral() with a malicious market.

removeCollateral - remove message pathway can be used to steal all the balance of the TapiocaOFT and mTapiocaOFT tokens in case when their underlying tokens is native.

Attacker can be debited only the least possible amount (1) but send the share argument as the maximum possible value corresponding to the erc balance of (m)TapiocaOFT.

This is an issue that affects all the contracts that inherit from NonBlockingLzApp due to incorrect overriding of the lzSend function and lack of input validation and the ability to specify whatever adapterParams you…

All TOFT and USDO modules have public functions that allow an attacker to supply an address module that is later used as a destination for a delegatecall.

Exercise option cross chain message in the (m)TapiocaOFT will always revert in the destination, but works in the source chain, where it debits the funds from users.

The function retrieveFromStrategy is used to trigger a removal of TOFT tokens from a strategy on a different chain. The function takes the parameter from, which is the account whose tokens will be retrieved.

All the ETH in the destination chain where sendFrom() is called is sent to the refundAddress in the LzCallParams.

The TapiocaOFT.sol contract allows users to wrap ERC20 tokens into an OFTV2 type contract to allow for seamless cross-chain use.

Status: Sponsor Confirmed

Oracle price of every market is updated on every action involving directly that market.

When doing a flashloan action in the BaseRouter.sol the beneficiary isn't compared with the beneficiary of the previous action, and the variable gets replaced with the beneficiary for the flashloan, this can lead to a…

The BaseRouter implements a mechanism to prevent tokens from being stuck that checks the balance of the router before and after the actions and ensures it remains equal.

totalAssets(...) and totalDebt(...) are fetched from the set of providers, which can be changed.

The rebalancer in the borrowing vault receives a fee for providing debtAsset to repay the originating provider and send to the destination provider.

Uniswap sets a deadline to limit arbitrage opportunities if the swap does not get included right away. If the swap specifies a deadline of block.timestamp, then the swap transaction can be included in any block.

In executeFailedWithUpdatedArgs(...), the allowedCaller can steal all assets available on the ConnextHandler by calling executeFailedWithUpdatedArgs(...) again after the xBundle(...) call.

In the BorrowingVault.sol the function previewMintDebt(...) is supposed to take an amount of shares and turn them into an amount of debt.

The BaseRouter interacts with the vault and some actions require tokens being pulled from the BaseRouter to the vault, which means that the BaseRouter must first approve the vault.

The BaseRouter checks if the initial balance of the tokens interacted with is the same as the end balance. However, for vaults, it only checks the balance of the underlying asset, not the shares themselves.

Currently the borrowing vault enables liquidating a user's position 50% if the debt/assets ratio is above liqRatio, but below liqRatio / FULLLIQUIDATIONTHRESHOLD.

The borrowing vault enables anyone to payback the debt of another user. Thus, attackers can frontrun a user payback transaction, payback 1 wei and stop users from paying back their debts.

BaseVault stores users allowances as underlying assets allowances. When a user calls, for example, approve(...), it converts the shares amount in the argument to a corresponding asset amount.

The BaseRouter checks if the balances of the assets remain the same after the xBundle(...) execution.

crossTransfer(...) has a beneficiary check such that the receiver of the funds in the destination chain must be the same as the previous beneficiary (most likely the msg.sender or the ConnextRouter).

The ConnextRouter places the xBundleConnext(...) call on a try/catch block to prevent failed actions to make the transaction revert and the funds being given to Connext.

The ConnextRouter allows users to bridge items from one chain to another and perform arbitrary actions there.

The ConnextRouter allows anyone to call xReceive(...) and the amount of assets the user provided is the balance of the contract. Thus, users can use this functionality to sweep any existing funds into their wallets.

Connext implements a mechanism of adjusting/forcing parameters in the bridged assets which can be triggered by a delegate.

Withdraw and borrow actions in the BaseRouter require the owner to increase the approval allowances.

In convertDebtToShares(...), if totalDebt(...) is 0, it reverts. This means that, if someone decides to repay the full vault debt, this function would always revert.

The BaseVault implements ERC5143, an extension of ERC4626. Thus, it should try to match every requirement of the ERC.

handleSwapAction(...) allows sending funds to the Flasher. This should be to pay for a taken flashloan.

Users make flashloans by calling xBundle(...) with a flashloan action.

This scenario only happens if the router in the destDomain has the same address as the router in the destDomain. Else, the beneficiary check will fail.

When the ConnextRouter message fails, it records the message in the ConnextHandler and sends it the funds.

When changing important addresses, it's best to use additional safety measures to prevent wrong addresses from being set. When the address to change is guaranteed to be a smart account, it accepts the new role.

Currently issue 3S-FUJI-M07 means that the deposit action can't be performed on a cross transfer at all.

An inflation attack occurs because attackers manipulate the denominator of the assets calculation by increasing totalAssets(...), which when calculating the shares as shares = assets supply / totalAssets(...), would…

maxRedeem(...) enables users to know how many shares they can redeem. In the case of the borrowing vault, it depends on the amount of debt a user has taken, which is taken into account in computeFreeAssets(...).

When using SafeERC20, it's common to implement with using SafeERC20 for IERC20. Take a look at the openzeppelin code. Addressed here: Fujicracy/fuji-v2648

The entrypoint in BaseFlasher is keccak256(abi.encode(data)) Remove abi.encode and set it as keccak256(data) Addressed here: Fujicracy/fuji-v2614

Storing the failed message itself is very expensive. Store the hash and then when calling executeFailedWithUpdatedArgs(...) send the txn as an argument and match it against the stored hash.

In ConnextHandler.sol, in order to record a failed transaction it is necessary to know the next nonce in the mapping, this involves a for cycle with a storage read that expends gas.

Users can bridge assets by inserting an Action.XTransferWithCall action, with calldata specified in the args field.

In the ConnextHandle, executeFailedWithUpdatedArgs(...), the failed tx executed field is set to true.

Before each transfer call, if the amount is 0, it should be skipped. This is also true for SafeERC20.safeTransfer(...) and SafeERC20.safeTransferFrom(...). An example would be Addressed here: Fujicracy/fuji-v2652

xBundle(...) and xReceive(...) make several external calls and check the balances before and after these calls. It's safer to include nonReentrant modifiers to prevent reentrancy.

The ConnextRouter does not check, in the crossTransfer(...) and crossTransferWithCalldata(...) functions, if the receiver and routerByDomain[destDomain] equal address(0).

checkNoBalanceChange(...) has a for cycle that goes through all tokens.

In handleSwapAction(...) the function checks if both the receiver and sweeper are the beneficiary given certain conditions.

Hardcoded values are hard to track and constants should be used instead.

tempTokenToCheck checks that the balance of the ConnextRouter does not change after having received tokens in xReceive(...) .

On the BorrowingVault.sol contract in the function withdraw(...) the if statement checks the following conditions: if (totalDebt == 0 && supply 0 && supply totalDebt) If the first two conditions are true the third will…

If a call reverts in the first if statement, the following else is not required. Replace with Addressed here: Fujicracy/fuji-v2625 and Fujicracy/fuji-v2622

In BaseVault.sol several functions receive a variable as a parameter then save it in memory and immediately use it to make a call without making any changes.

In BaseRouter, bundleInternal(...), the tokensToCheck array is updated in tokensToCheck = addTokenToList(token, tokensToCheck);.

Reduces decentralization significantly and discourages delegates with less token power to vote. The current math gives delegates rewards based on the square of their votes.

In MapleLoanInitializer, the address of the fee manager is not whitelisted. Exploit scenario ● Borrower agrees with delegate on the terms and creates a loan with the correct fee manager;

The delegate origination fee can be set to any value at any time and anyone can deploy a loan, so the delegate can receive all the funds of a loan.

The current PoolManager contract allows the pool manager to change the withdrawalManager contract without any checks using the setWithdrawalManager() function.

The platform service fee is set on loan deployment; however, when a pool is deployed, its platform service fee is 0 (has not been set yet), so a borrower could deploy a loan right after a pool deployment and have 0…

It's possible to stop loans from being funded by calling returnFunds().

A Pool Delegate should not be allowed to be a borrower otherwise they can leave with the funds at any time with no questions asked.

Using the function setDelegateManagementFeeRate() the pool manager can set an unreasonably large delegateManagementFeeRate (all the way up to 100%) at any time, stopping payments from reaching the pool (i.e.

In MapleGlobals, activatePoolManager(), there is no check that the Pool Manager and Pool Delegate are valid, so the governor can mistakenly set malicious addresses as activated.

In fixed term loans, a DoS attack is possible if assets are transferred into the MapleLoan before the loan is funded.

The usual flow is to impair a loan and then call triggerDefault() followed by finishCollateralLiquidation();

Pool delegates are allowed to change the withdrawal manager address at any time using the setWithdrawalManager() function.

In the previous maple-core-v2 code, any address could be chosen for the refinancer when proposing new terms.

Terms can be proposed with duplicated calls, which could lead to mistakes or phishing attacks. Send the calls ordered by selector and check that the next selector is strictly bigger than the previous.

Could lead to loss of funds if someone sends a zero address by mistake. Add a transfer to 0 address check. Addressed in the following PR:

The code allows for the Borrower to choose a different fundsAsset than the lender.

By using the fallback function in the MapleLoanInitializer to initialize a loan (both for open term and for fixed term) it allows the borrower to choose a vanity address to end up in any public function of the…

In critical functions that may affect the decision of users to stay invested in the protocol (for example functions that set certain pool terms, fees, upgrades, etc.), there are no timelocks implemented, which will give…

In MapleGlobals.sol, in unscheduleCall(), the CallUnscheduled event is emitting dataHash (the hash of the callData) which is never checked, and could be different than the one it is unscheduling.

Some fees or delegate cover information are set in the globals. When fetched, there is no check that the values for a specific pool or loan manager have been set, so the default 0 value is assumed.

The setOpenToPublic(...) function only enables setting openToPublic to true, so if a delegate sets a permissioned pool to open by mistake, it's impossible to undo it. Add setOpenToPublic as an argument.

Whenever addresses change in the code, it's important to be careful about the possibility of setting the wrong address.

In the MapleLoan contract, the following variables are loaded from storage multiple times: ● acceptNewTerms(): lender is loaded twice from storage. ● acceptNewTerms(): borrower is loaded from storage three times.

On the MapleLoanStorage contract, a storage slot can be reduced by packing the last variable: platformServiceFeeRate (a uint64) with one of the addresses. Pack the variables into a single storage slot.

On open-term-loan/MapleLoan.sol, line 84 of acceptNewTerms(), there is a require to check that the refinance commitment according to the terms sent by the user is valid and to do so refinanceCommitment is loaded from…

On line 221 the factory address is assigned from IMapleProxied(msg.sender).factory(). However, on line 225, IMapleProxied(msg.sender).factory() is called again instead of using the already fetched factory.

The mapping isLoan is only modified in the respective MapleLoanFactory.sol contract which extends MapleProxyFactory.sol.

In the LoanManager contract, two changes can be made to reduce gas usage and improve readability: In function accountForLoanImpairmentRemoval(): loading the payment struct from storage should only be performed after the…

The LoanManager contract has the following code: In this code, functions accountForLoanImpairment(loan) and accountForLoanImpairmentAsGovernor(loan) add unneeded redundancy and make the code more cluttered, therefore…

Solidity allows for their implementation of native custom errors that is more gas-efficient than using a string within the require function to explain to the user where the call reverted (the way it's implemented in the…

On open-term-loan in function makePayment(), first the payment breakdown is called and after that the loan is checked to be active. getPaymentBreakdown() does several storage reads.

In the PoolManager contract, function setIsLoanManager(): variable loanManagerList.length could be held in memory preventing multiple storage reads during the for loop.

There are some discrepancies in the way that checks for access control/requires are done throughout the code base.

Fixed and Open term loans implement SetPendingLender and AcceptLender; however, the corresponding LoanManagers don't.

Throughout the repository, a total of 14 comments and 21 wiki errors were reported. Fix the corresponding errors. Addressed in the following PRs:

There is a typo in the name of the function setContactPause(). Change setContactPause() to setContractPause(). Addressed in the following PR: maple-labs/globals-v2-private61

Time-sensitive contracts will be impacted if deployed on zkSync.

Attacker can run executeDraw() in Lottery.sol, receive random numbers and then buy tickets with known numbers in one block.

A counterfactual wallet can be used by pre-generating its address using the SmartAccountFactory.getAddressForCounterfactualWallet function.

When creating a minipool the node operator is required to put up a collateral in GGP, the protocol token. The amount of GGP collateral needed is currently calculated to be 10% of the AVAX staked.

When a node operator creates a minipool they pass which duration they want to stake for. There is no validation for this field so they can pass any field: Later when staking is done.

Function syncRewards() distributes rewards to TokenggAVAX holders, it linearly distribute cycle's rewards from block.timestamp to the cycle end time which is next multiple of the rewardsCycleLength (the end time of the…