Organization
- @seamless
Engagement Type
Cantina Reviews
Period
-
Repositories
Researchers
Findings
High Risk
1 findings
1 fixed
0 acknowledged
Low Risk
3 findings
3 fixed
0 acknowledged
Informational
3 findings
3 fixed
0 acknowledged
Gas Optimizations
2 findings
1 fixed
1 acknowledged
High Risk1 finding
Unauthorized Transfer of User-Approved ERC20 Tokens via LeverageRouter
Description
In
_depositAndRepayMorphoFlashLoan&_redeemAndRepayMorphoFlashLoaninternal functions, it allows arbitrary external calls to the any addresses which are passed asCall[] calldata swapCallsfromdepositorredeementrypoint.for (uint256 i = 0; i < params.swapCalls.length; i++) { // slither-disable-next-line unused-return Address.functionCallWithValue( params.swapCalls[i].target, params.swapCalls[i].data, params.swapCalls[i].value ); }Since
LeverageRouteris used for minting and redeeming leverage tokens by end users, It needs to have ERC20 allowance for minting shares and leverage tokens allowance for redeeming. These are approved just in time before actual mint / redeem transaction or may have infinite allowance if given by user.However, A malicious actor can drain all funds by adding a
transferFromcall inswapCallsinput to deposit / redeem function on ERC20 from user who has approved, andtoaddress being their controlled addressOther similar paths exist to extract to steal the approved funds & to take allowance so that future funds could be stolen such as:
- calling
morpho.flashLoanfrom arbitrary call which triggers callback with data which is not sanitized. - calling
permit2to take the infinite token allowance of router where spender is attacker controlled address.
This is especially more exploitable on chains which public mempool such as ethereum mainnet.
Proof of Concept
function test_Alice_steals_bob_approval() public { address alice = makeAddr("alice"); uint bobsCollateral = 123 ether; // Execute the deposit deal(address(collateralToken), address(this), bobsCollateral); collateralToken.approve(address(leverageRouter), bobsCollateral); ILeverageRouter.Call[] memory aliceStealCall = new ILeverageRouter.Call[](1); aliceStealCall[0] = ILeverageRouter.Call({ target: address(collateralToken), data: abi.encodeWithSelector(IERC20.transferFrom.selector, address(this), address(alice), bobsCollateral), value: 0 }); // Alice starts with no collateral token assertEq(collateralToken.balanceOf(alice), 0); _mockLeverageManagerDeposit(0, 0, 0, 0); vm.prank(alice); leverageRouter.deposit(leverageToken, 0, 0, 0, aliceStealCall); // now Alice has Bob's collateral assertEq(collateralToken.balanceOf(alice), bobsCollateral); }Recommendation
There are two main ways of tackling this:
- All externals calls should be fully validated for address and calldata being trusted and whitelisted to ensure there are no risks. Anything which is in path of morpho flashloan callback should check for re-entrancy
- Use a separate contract to make untrusted externals which doesn't hold any asset, approvals or privilleged roles outside of a single transaction context
- calling
Low Risk3 findings
PricingAdapter doesn't account for fee adjusted supply, leading to potential overpricing
Severity
- Severity: Low
≈
Likelihood: High×
Impact: Low Submitted by
Denis Miličević
Description
The
PricingAdaptercontract is intended to provide pricing denominations of the leveraged tokens in their respective collateral or debt tokens. The logic for calculating this utilizes the leveraged tokenstotalSupply()function, which in cases that a management fee is active, fails to account for the accrued fees on the supply, and thereby underreports the actual supply present, which results in an overpriced output for contracts or off-chain utilities depending on the pricing.Recommendation
Utilize the
getFeeAdjustedTotalSupply(ILeverageToken token)function instead to provide a more accurate pricing output.Ceiling rounding in _getAccruedManagementFee can yield a much higher effective fee being charged than set
Severity
- Severity: Low
≈
Likelihood: Low×
Impact: Medium Submitted by
Denis Miličević
Description
In almost all cases, with the current logic implemented in
_getAccruedManagementFee, the set management fee, is basically the minimum fee that would be effectively charged. In most instances, it will be at least fractionally greater, under exceptional circumstances, the effective fee could end up being orders of magnitude higher through the lifetime of the token, with those scenarios being a low supply token resultant from a donation attack which would be further amplified by a low management fee.Recommendation
Switch the logic to do a floor rounding, which will now make the set fee be the maximal possible fee chargable by the protocol under ideal scenarios. In reality it will be slightly lower, in the worst case of an exceptional scenario, where all users of the contract purposefully abuse the floor rounding by specifically timing all transactions, and any users, including the team have their transactions censored, the effective fee could be cut in half, which is a much lower range than in the prior exceptional cases, which are also more likely to occur, this noted scenario is very unlikely to occur. This change would effectively improve the consistency and accuracy of the management fee.
There will need to be more logic changes, as the
lastManagementFeeAccrualTimestamp[token]storage variable for the token cannot be updated at all times, but only in the case the fee is actually charged, and management fee is actually set, as we don't want duration for fees increasing while there is no fee set, allowing potentially for a "fee bomb" to be set all at once. Essentially a check formanagementFee != 0 && feeShares == 0which early returns and doesn't update the fee accrual timestamp if the condition is met._computeFeesForNetShares fails to account for additional rounding step in reversal of _computeFeesForGrossShares
Severity
- Severity: Low
≈
Likelihood: High×
Impact: Low Submitted by
Denis Miličević
Description
The logic in
_computeFeesForNetSharesdiscounts an additional rounding step that occurs in_computeFeesForGrossShares. This leads to additional off-by-one cases betweenpreviewRedeemandpreviewWithdrawoutputs on thetreasuryFeeaction data.An example showcasing this:
Preview redeem 10 shares returns: 8 collateral 4 debt 10 shares 1 token fee share 1 treasury fee share Preview withdraw 8 shares returns: 8 collateral 4 debt 9 shares 1 token fee share 0 treasury fee shareWith the same equivalent inputs they should ideally match, otherwise the lower fee call of withdraw could be detrimental to the treasury.
Recommendation
Account for the additional rounding step by switching the logic to:
grossShares = Math.mulDiv(Math.mulDiv( netShares, MAX_BPS, (MAX_BPS - tokenActionFeeRate), Math.Rounding.Ceil ), MAX_BPS, MAX_BPS - treasuryActionFeeRate, Math.Rounding.Ceil);This solution resolves the above cases mismatch, but there is still some precision loss which results in a mismatch of 1 for other cases between
previewRedeemandpreviewWithdraw, however, from fuzz tests this logic halves the occurrance those cases compared to the current logic.
Informational3 findings
Public function with internal variant, should just be set to external
Severity
- Severity: Informational
Submitted by
Denis Miličević
Description
The function
convertCollateralToSharesin theLeverageManagercontract has its visibility set to public, although internally it is accessed via the internal variant_convertCollateralToShareswhich it calls itself into as well. This is likely an oversight or typo, as all the other similar functions within the contract follow the external/internal pattern already.Recommendation
Set its visibility specifier to external, to avoid issues arising between internal use of the internal or external facing function.
New mints are possible post complete liquidation
Severity
- Severity: Informational
Submitted by
Om Parikh
Description
one could deposit directly on lending adapter on behalf of lending adapter when
totalSupply !=0 && totalCollateral == 0so that both becomes non-zero now. this undermines the stated assumption in commnets and whoever deposits can control (and possibly skew) the exchange rate.However, there are no major implications as shares were already worth nothing after liquidation.
Recommendation
- The comments (and/or any other related docs) should be updated if deemed important to outline this scenario.
- If it should not be possible to mint at all in this case, it should be enforced via some stricter mechanism.
PricingAdapter should be discouraged for on-chain use
Severity
- Severity: Informational
Submitted by
Om Parikh
Description
getLeverageTokenPriceAdjustedreturns oracle price adjusted value.for e.g: If oracle = DAI / USD, then
oraclePrice=0.99e8debt token is DAI,baseAssetDecimalsis 18, then adjusted price can round down to 0 or be extremely imprecise depending on precision of base asset and oracle used.Recommendation
Consider documenting that it should be only used for offchain batching of calls and not for on-chain callers / integrations as the precision & rounding direction used may not be suitable for all use cases.
Gas Optimizations2 findings
Struct in FeeManager has variables that could be tightly packed
State
- Acknowledged
Severity
- Severity: Gas optimization
Submitted by
Denis Miličević
Description
The
FeeManagerStoragestruct in theFeeManagercontract has 2 separate mappings pointing touint256types that have code paths leading to storage loading and storing within the same call. Additionally, the types can safely be bounded to smalleruinttypes.Recommendation
The mappings should be condensed into a single mapping, pointing to a struct that contains the
managementFeeandlastManagementFeeAccrualTimestamp. A safe type formanagementFeeisuint16, anduint120forlastManagementFeeAccrualTimestamp, which can save aSSTOREorSLOADopcode when these are both accessed._getAccruedManagementFee can be optimized for on-chain callpaths
Severity
- Severity: Gas optimization
Submitted by
Denis Miličević
Description
The
_getAccruedManagementFeeattempts some operations that are redundant when accessed throughdeposit,mint,redeem,withdraw, which is wasting gas, as thechargeManagementFeefunction will have already run through these operations and appropriately adjust thetotalSupply()with fees.Recommendation
Have the function early return 0, skipping unnecessary operations, and by re-ordering a portion, allows the skipping of an additional unnecessary warm storage load, saving at least 100 gas.
from
function _getAccruedManagementFee(ILeverageToken token, uint256 totalSupply) internal view returns (uint256) { uint256 managementFee = getManagementFee(token); uint120 lastManagementFeeAccrualTimestamp = getLastManagementFeeAccrualTimestamp(token); uint256 duration = block.timestamp - lastManagementFeeAccrualTimestamp; uint256 sharesFee = Math.mulDiv(managementFee * totalSupply, duration, MAX_BPS * SECS_PER_YEAR, Math.Rounding.Ceil); return sharesFee; }to
function _getAccruedManagementFee(ILeverageToken token, uint256 totalSupply) internal view returns (uint256) { uint120 lastManagementFeeAccrualTimestamp = getLastManagementFeeAccrualTimestamp(token); uint256 duration = block.timestamp - lastManagementFeeAccrualTimestamp; if (duration == 0) return 0; uint256 managementFee = getManagementFee(token); uint256 sharesFee = Math.mulDiv(managementFee * totalSupply, duration, MAX_BPS * SECS_PER_YEAR, Math.Rounding.Ceil); return sharesFee; }