Issue H-1: Rebalancer can steal funds from markets by sending to custom receiver through Everclear Bridge
Source: #124
Angry_Mustache_Man, ExtraCaterpillar, SafetyBytes, Sevyn, Ziusz, axelot, bulgari, cergyk, elolpuer
The rebalancer contract is called on sendMsg with a message encoded as bytes (_msg.message). This message is specific for the bridge contract, and in the case of EverclearBridge it is of the form IntentParams:
struct IntentParams {
uint32[] destinations;
//@audit receiver is left unchecked
bytes32 receiver;
address inputAsset;
bytes32 outputAsset;
uint256 amount;
uint24 maxFee;
uint48 ttl;
bytes data;
IFeeAdapter.FeeParams feeParams;
}We notice that the parameter receiver is left entirely unchecked, and the rebalancer EOA can provide an address controlled by it, stealing the user funds which should be send for rebalancing.
EverclearBridge.sol#L111-L121:
(bytes32 id,) = everclearFeeAdapter.newIntent(
params.destinations,
params.receiver,
params.inputAsset,
params.outputAsset,
params.amount,
params.maxFee,
params.ttl,
params.data,
params.feeParams
);Check that params.receiver is a valid market on the destination chain
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: malda-protocol/malda-lending#103
CergyK
Fix looks good, now params.receiver is forced to be equal to _market
Source: #81
cergyk
In the mToken contract, forked from compound v2, the same wrong direction rounding is introduced in the __redeem function:
if (redeemTokensIn > 0) {
/*
* We calculate the exchange rate and the amount of underlying to be redeemed:
* redeemTokens = redeemTokensIn
* redeemAmount = redeemTokensIn x exchangeRateCurrent
*/
redeemTokens = redeemTokensIn;
redeemAmount = mul_ScalarTruncate(exchangeRate, redeemTokensIn);
} else {
/*
* We get the current exchange rate and calculate the amount to be redeemed:
* redeemTokens = redeemAmountIn / exchangeRate
* redeemAmount = redeemAmountIn
*/
>> redeemTokens = div_(redeemAmountIn, exchangeRate); //@audit should round up, as this is the amount of tokens pulled from user
redeemAmount = redeemAmountIn;
}This wrong direction of rounding leads to pull less shares from the user while the amount sent out is the same as requested. In most cases the losses should be negligible (1 wei of shares per operation), because the division operates at 1e18 precision, and the exchange rate starts as a small value initially (0.02).
We will show that the first depositor is able to increase the exchange rate by an arbitrary factor, by using the rounding against him during mint and redeem(shares), when supply is initially zero.
As can be seen in the mint function:
uint256 mintTokens = div_(actualMintAmount, exchangeRate);
require(mintTokens >= minAmountOut, mt_MinAmountNotValid());
// avoid exchangeRate manipulation
if (totalSupply == 0) {
//@audit 1000 of shares are minted to the zero address
totalSupply = 1000;
accountTokens[address(0)] = 1000;
//@audit underflow if initial mint is below 1000
mintTokens -= 1000;
}When the totalSupply is zero, at least 1000 shares have to be minted initially, which means that 20 wei of token has to be supplied (due to the 0.02 initial exchange rate).
Then the first depositor repeats the steps:
- Mint by providing 1 wei of underlying (mints 50 shares to the depositor)
- Redeem 25 shares *2 -> sends 0 underlying to the depositor
each time these steps are repeated, the totalUnderlying of the market is increased by 1 while totalSupply is unchanged. This means the exchangeRate is increasing. We need to repeat this 980 times to reach echangeRate == exp
As a side note, the host chain here is
Linea, and although repeating these steps many times costs a lot of gas (around 50M), the gas would be very cheap; Such a step of 1000 iterations only costs around 0.20$ at current ethereum price. Also please note that the gas limit by block is very large (2B).
When exchangeRate > exp, the process is simpler we only need to provide 1 wei and 0 shares are minted. From now on, we double the exchange rate at each step of 1000 iterations:
Indeed when exchange rate is X*exp, we can call mint with X wei, which will mint 0 shares.
With this in mind and taking the example of USDC, reaching an exchange rate value of 1e6*exp, would take approx 20 iterations and cost ~5$ of gas, plus the cost of donating 1000$ of token to the market.
The attacker just has to wait for new depositors to supply into the market, and then he can call redeemUnderlying 1e6 by 1e6, which will burn zero shares.
Consider fixing the rounding in redeem, make the division by exchange rate round up: mToken.sol#L614-L630:
} else {
/*
* We get the current exchange rate and calculate the amount to be redeemed:
* redeemTokens = redeemAmountIn / exchangeRate
* redeemAmount = redeemAmountIn
*/
- redeemTokens = div_(redeemAmountIn, exchangeRate);
+ redeemTokens = divUp_(redeemAmountIn, exchangeRate);
redeemAmount = redeemAmountIn;
}sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: malda-protocol/malda-lending#117
CergyK
Fix looks good, redeem underlying now rounds up, in favor of the protocol
Source: #86
This issue has been acknowledged by the team but won't be fixed at this time.
Bobai23, Kvar, LeFy, cergyk, dan__vinci
The borrow rate is computed in JumpRateModelV4 as a function of the utilisation:
JumpRateModelV4.sol#L140-L150:
function getBorrowRate(uint256 cash, uint256 borrows, uint256 reserves) public view override returns (uint256) {
uint256 util = utilizationRate(cash, borrows, reserves);
if (util <= kink) {
return util * multiplierPerBlock / 1e18 + baseRatePerBlock;
} else {
uint256 normalRate = kink * multiplierPerBlock / 1e18 + baseRatePerBlock;
uint256 excessUtil = util - kink;
return excessUtil * jumpMultiplierPerBlock / 1e18 + normalRate;
}
}Utilization rate being defined as:
JumpRateModelV4.sol#L127-L135:
function utilizationRate(uint256 cash, uint256 borrows, uint256 reserves) public pure override returns (uint256) {
if (borrows == 0) {
return 0;
}
return borrows * 1e18 / (cash + borrows - reserves);
}We notice that utilizationRate can be greater than 1e18, when cash + borrows - reserves < borrows. In the following section, we will show a scenario where the ratio can become very big.
Assuming a 18 decimals market:
-
Alice the first depositor can mint for 1e18 tokens, and borrow it all (by having additional collateral in another market):
- cash = 0
- totalBorrows = 1e18
- reserves = 0
-
Alice waits for a few blocks for some interest to accrue:
- cash = 0
- totalBorrows = 1e18 + 1e12
- reserves = 5e11 (assuming 50% reserve factor)
-
Alice repays totalBorrows minus reserves minus 1:
- cash = 1e18 + 5e11 - 1
- totalBorrows = 5e11 + 1
- reserves = 5e11
-
Alice redeems the total supply
- cash = 0
- totalBorrows = 5e11 + 1
- reserves = 5e11
utilization rate is very big: 5e11. As a result the getBorrowRate is bigger than 0.0005e16 (initial max borrow value):
constructor() {
borrowRateMaxMantissa = 0.0005e16;
}As a result the market is completely bricked, because _accrueInterest always reverts:
mTokenStorage.sol#L351-L356:
//@audit in _accrueInterest():
/* Calculate the current borrow interest rate */
uint256 borrowRateMantissa =
IInterestRateModel(interestRateModel).getBorrowRate(cashPrior, borrowsPrior, reservesPrior);
if (borrowRateMaxMantissa > 0) {
require(borrowRateMantissa <= borrowRateMaxMantissa, mt_BorrowRateTooHigh());
}Indeed _accrueInterest is called for all operations, even for updating the interestRateModel.
Consider bounding the utilization to be at most 1e18 (100%), which would avoid having degenerate borrow rate values.
Source: #91
10ap17, Angry_Mustache_Man, LeFy, WillyCode20, cergyk, elolpuer, pashap9990, vangrim
Migrator.sol implements a slippage protection:
// 2. Mint mTokens in all v2 markets
for (uint256 i; i < posLength; ++i) {
Position memory position = positions[i];
if (position.collateralUnderlyingAmount > 0) {
//@audit compute slippage value
uint256 minCollateral =
position.collateralUnderlyingAmount - (position.collateralUnderlyingAmount * 1e4 / 1e5);
ImErc20Host(position.maldaMarket).mintOrBorrowMigration(
true, position.collateralUnderlyingAmount, msg.sender, address(0), minCollateral
);
}
}Unfortunately, the slippage value computed uses underlying tokens, whereas the shares amount should be used. Since exchange rate should be close to 0.02, the slippage parameter is wrong by at least an order of magnitude.
Underlying amount should be divided by exchange rate to have a correct slippage value: Migrator.sol#L90-L100:
// 2. Mint mTokens in all v2 markets
for (uint256 i; i < posLength; ++i) {
Position memory position = positions[i];
if (position.collateralUnderlyingAmount > 0) {
- uint256 minCollateral =
- position.collateralUnderlyingAmount - (position.collateralUnderlyingAmount * 1e4 / 1e5);
+ uint256 minCollateral =
+ div_(position.collateralUnderlyingAmount - (position.collateralUnderlyingAmount * 1e4 / 1e5), _exchangeRate);
ImErc20Host(position.maldaMarket).mintOrBorrowMigration(
true, position.collateralUnderlyingAmount, msg.sender, address(0), minCollateral
);
}
}sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: malda-protocol/malda-lending#120
CergyK
Fix looks good, underlying was converted to shares using exchangeRate
Source: #96
0xEkko, 0xKann, 5am, 8olidity, JeRRy0422, Kvar, PeterSR, TopStar, ZanyBonzy, Ziusz, blockace, bulgari, cergyk, dimulski, gh0xt, joicygiore, kelvinclassic11
The mTokenGateway exposes the outHere endpoint to allow withdrawal on an extension chain. This endpoint is protected by two ifNotBlacklisted modifiers:
/**
* @inheritdoc ImTokenGateway
*/
function outHere(bytes calldata journalData, bytes calldata seal, uint256[] calldata amounts, address receiver)
external
notPaused(OperationType.AmountOutHere)
ifNotBlacklisted(msg.sender)
ifNotBlacklisted(receiver)
{Unfortunately both can be bypassed, because:
-
msg.sendercan be any address inallowedCallersfor the_senderin the journal data -
receiver value passed to the endpoint is overwritten by
_senderin_outHere:function _outHere(bytes memory journalData, uint256 amount, address receiver) internal { (address _sender, address _market,, uint256 _accAmountOut, uint32 _chainId, uint32 _dstChainId,) = mTokenProofDecoderLib.decodeJournal(journalData); // temporary overwrite; will be removed in future implementations receiver = _sender;
So a blacklisted user can still use outHere to withdraw their funds.
Add an additional check for _sender to not be in blacklist:
function _outHere(bytes memory journalData, uint256 amount, address receiver) internal {
(address _sender, address _market,, uint256 _accAmountOut, uint32 _chainId, uint32 _dstChainId,) =
mTokenProofDecoderLib.decodeJournal(journalData);
// temporary overwrite; will be removed in future implementations
receiver = _sender;
+ require (!blacklistOperator.isBlacklisted(_sender), mTokenGateway_UserBlacklisted());sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: malda-protocol/malda-lending#112
CergyK
Fix looks good
Source: #123
cergyk
The rebalancer contract is called on sendMsg with a message encoded as bytes (_msg.message). This message is specific for the bridge contract, and in the case of EverclearBridge it is of the form IntentParams:
struct IntentParams {
uint32[] destinations;
bytes32 receiver;
address inputAsset;
bytes32 outputAsset;
uint256 amount;
uint24 maxFee;
uint48 ttl;
bytes data;
IFeeAdapter.FeeParams feeParams;
}We notice that multiple destination chains can be provided (destinations); Out of these chains, it is only checked that one is the _dstChainId provided as argument of sendMsg:
bool found;
for (uint256 i; i < destinationsLength; ++i) {
if (params.destinations[i] == _dstChainId) {
found = true;
break;
}
}
require(found, Everclear_DestinationNotValid());However, the intent is created on Everclear using all the provided destinations, and can be fulfilled on any of the chains of the list. If it is fulfilled on a chain that Malda does not handle, the funds are lost.
destinations should be checked to be of length 1, and that destinations[0] == _dstChainId
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: https://github.com/malda-protocol/malda-lending/pull/119/files
CergyK
Fix looks good, only 1 destination chain allowed now
Issue M-6: EverclearBridge does not pull tokens from the Rebalancer, causing all rebalancing operations to fail
Source: #177
0xDemon, 0xmechanic, 0xsai, 10ap17, Angry_Mustache_Man, BusinessShotgun, Cybrid, ExtraCaterpillar, IvanFitro, Sa1ntRobi, SafetyBytes, ZanyBonzy, ZeroTrust, Ziusz, bube, bulgari, dan__vinci, davies0212, maigadoh, oxelmiguel, vangrim
The EverclearBridge contract does not transfer tokens from the Rebalancer contract before attempting to perform bridging operations. It assumes custody of tokens without actually calling safeTransferFrom to pull tokens from the Rebalancer. As a result, the bridge contract does not have the required tokens, causing all rebalancing attempts to fail.
In EverclearBridge.sol, the sendMsg function does not call IERC20(_token).safeTransferFrom(msg.sender, address(this), params.amount) to transfer tokens from the Rebalancer to itself. Without this transfer, the bridge cannot perform any bridging logic that requires token custody.
- The Rebalancer approves the bridge contract to spend tokens.
- The Rebalancer calls
sendMsgon the bridge contract without transferring tokens.
None.
- The Rebalancer approves the bridge contract.
- The bridge contract attempts to perform bridging logic but fails due to lack of token custody.
- No tokens are bridged and rebalancing fails.
All rebalancing operations using the bridge contract will fail, resulting in a complete denial of service for cross-chain liquidity movement. This can halt protocol operations that depend on rebalancing and may lead to liquidity fragmentation or loss of protocol functionality.
No response
Update the EverclearBridge contract to call safeTransferFrom and pull the required tokens from the Rebalancer before proceeding with bridging logic.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: malda-protocol/malda-lending#105
CergyK
Fix looks good. Tokens are now pulled from Rebalancer into EverclearBridge
Source: #234
0x213, 0xBlackie, 0xEkko, 0xShoonya, 0xSolus, 0xapple, 0xmechanic, 10ap17, 5am, 7, 8olidity, BADROBINX, Bizarro, Cybrid, DeveloperX, EQUATION, HeckerTrieuTien, Ivcho332, KiroBrejka, LeFy, MacroWang, PratRed, RaiseUp, Sparrow_Jac, WillyCode20, Yaneca_b, ZanyBonzy, Ziusz, axelot, coin2own, dan__vinci, davies0212, djshaneden, dreamcoder, elolpuer, elyas, farismaulana, gh0xt, har0507, harry, magbeans9, mahdifa, maxim371, onudasatoshi, oxelmiguel, oxwhite, pollersan, sheep, softdev0323, swarun, teoslaf1, v10g1, vangrim, who_is_rp, yoooo, zach223
Using stale TransferInfo in the Rebalancer::sendMsg function will cause a false Rebalancer_TransferSizeExcedeed revert for EOA rebalancers as in normal use flow, the contract checks the old transfer size even after resetting it for a new time window, leading to unexpected reverts and potential disruption of rebalancing operations.
TransferInfo memory transferInfo = currentTransferSize[_msg.dstChainId][_msg.token];
uint256 transferSizeDeadline = transferInfo.timestamp + transferTimeWindow;
if (transferSizeDeadline < block.timestamp) {
currentTransferSize[_msg.dstChainId][_msg.token] = TransferInfo(_amount, block.timestamp);
} else {
currentTransferSize[_msg.dstChainId][_msg.token].size += _amount;
}
uint256 _maxTransferSize = maxTransferSizes[_msg.dstChainId][_msg.token];
if (_maxTransferSize > 0) {
require(transferInfo.size + _amount < _maxTransferSize, Rebalancer_TransferSizeExcedeed());
//@audit if `transferSizeDeadline < block.timestamp` than `transferInfo` won't be updated but we changed `currentTransferSize` to be `TransferInfo(_amount, block.timestamp)` so we will check using the old transfer size + amount instead in this case using only amount which will lead to unexpected DoS
}In Rebalancer.sol, the sendMsg function resets currentTransferSize[_msg.dstChainId][_msg.token] to TransferInfo(_amount, block.timestamp) when the time window expires, but still uses the old transferInfo.size for the max transfer size check (require(transferInfo.size + _amount < _maxTransferSize)), causing incorrect validation and potential DoS.
- In normal use, an authorized rebalancer performs transfers that bring the recorded
currentTransferSizeclose to the maximum allowed for a token and destination chain within the current time window. - Once the time window expires, the next transfer resets
currentTransferSizeto the new transfer amount with an updated timestamp. - However, during this next transfer, the contract still checks the sum of the old (stale) transfer size plus the new amount against the max limit, causing an unexpected revert and blocking valid transfers.
none
- An authorized rebalancer calls
sendMsgmultiple times within a singletransferTimeWindow, pushingcurrentTransferSize[_dstChainId][_token].sizeclose tomaxTransferSizes[_dstChainId][_token]. - The
transferTimeWindowexpires without further transfers, socurrentTransferSize[_dstChainId][_token]remains at a high value but stale. - On the first
sendMsgcall after the window resets, the contract resetscurrentTransferSizeto the new amount but still validates using the old (stale)transferInfo.size. - Because the validation uses the stale size, the contract rejects the transfer by reverting with
Rebalancer_TransferSizeExcedeed, even though the new window should allow it. - This causes a denial of service for rebalancing operations on that token and destination chain until the next window reset or state update.
The authorized rebalancers cannot execute valid rebalancing transfers for certain tokens and destination chains immediately after a transfer time window resets. This causes a denial of service in normal use flow, disrupting expected protocol operations without any malicious actor involved.
No response
No response
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: malda-protocol/malda-lending#138
CergyK
Fix looks good
Issue M-8: mErc20Host: It is not possible to permissionlessly call "external" endpoints when source chain is Eth mainnet, because l1Inclusion flag cannot be set to true
Source: #264
This issue has been acknowledged by the team but won't be fixed at this time.
LeFy, cergyk, pyk
We see that in order to submit a proof to the malda-lending contracts permissionlessly (user is not proof forwarder or batch proof forwarder), the l1Inclusion flag has to be set:
function _verifyProof(bytes calldata journalData, bytes calldata seal) internal view {
require(journalData.length > 0, mErc20Host_JournalNotValid());
// Decode the dynamic array of journals.
bytes[] memory journals = _decodeJournals(journalData);
// Check the L1Inclusion flag for each journal.
bool isSequencer = _isAllowedFor(msg.sender, _getProofForwarderRole())
|| _isAllowedFor(msg.sender, _getBatchProofForwarderRole());
if (!isSequencer) {
for (uint256 i = 0; i < journals.length; i++) {
> (,,,,,, bool L1Inclusion) = mTokenProofDecoderLib.decodeJournal(journals[i]);
> if (!L1Inclusion) {
> revert mErc20Host_L1InclusionRequired();
> }
}
}
// verify it using the IZkVerifier contract
verifier.verifyInput(journalData, seal);
}However if attempting to request a proof for data originating from the ETH mainnet, with l1Inclusion flag set, it will panic in viewcalls.rs, during the get_env_input_for_l1_inclusion_and_l2_block_number call:
pub async fn get_env_input_for_l1_inclusion_and_l2_block_number(
chain_id: u64,
is_sepolia: bool,
l1_inclusion: bool,
ethereum_block: Option<u64>,
fallback: bool,
) -> (Option<EvmInput<EthEvmFactory>>, Option<u64>) {
if !l1_inclusion {
// If L1 inclusion is not required, return None for both values
(None, None)
} else {
//@audit handle the l1_inclusion == true case
// Prepare the L1 RPC URL
let l1_rpc_url = get_rpc_url("ETHEREUM", fallback, is_sepolia);
// Determine the L1 block to use for inclusion
let l1_block = if is_linea_chain(chain_id) {
ethereum_block.unwrap()
} else {
if is_sepolia {
ethereum_block.unwrap() - REORG_PROTECTION_DEPTH_ETHEREUM_SEPOLIA
} else if !is_sepolia {
ethereum_block.unwrap() - REORG_PROTECTION_DEPTH_ETHEREUM
} else {
panic!("Invalid chain ID");
}
};
// Delegate to the appropriate helper based on chain type
if is_opstack_chain(chain_id) {
get_env_input_for_opstack_dispute_game(chain_id, l1_block, fallback).await
} else if is_linea_chain(chain_id) {
get_env_input_for_linea_l1_call(chain_id, l1_rpc_url, l1_block).await
} else {
//@audit if ETH mainnet && l1_inclusion, panic
> panic!(
> "L1 Inclusion only supported for Optimism, Base, Linea and their Sepolia variants"
> );
}
}
}It is impossible to carry some external actions on host in a permissionless way (caller is not sequencer), when source chain is Eth mainnet. The impact is even more acute since there is no clear path to trigger some external endpoints for execution by the sequencer (see issue about liquidateExternal).
No preconditions, the bug is inevitable
The process of validating the data (guest program, logic of which is in validators.rs) allows for validating a proof with l1Inclusion set to true for ETH_MAINNET, it only requires the env_input_eth_for_l1_inclusion to be Some (this value is not used anymore after determining validate_l1_inclusion).
malda-zk-coprocessor/malda_utils/src/validators.rs#L195:
let validate_l1_inclusion = env_input_eth_for_l1_inclusion.is_some();So we should just handle the ethereum case by returning a default ENV value:
// Delegate to the appropriate helper based on chain type
if is_opstack_chain(chain_id) {
get_env_input_for_opstack_dispute_game(chain_id, l1_block, fallback).await
} else if is_linea_chain(chain_id) {
get_env_input_for_linea_l1_call(chain_id, l1_rpc_url, l1_block).await
- } else {
- panic!(
- "L1 Inclusion only supported for Optimism, Base, Linea and their Sepolia variants"
- );
+ } else if is_ethereum(chain_id) {
+ Env::default();
+ } else {
+ panic!(
+ "L1 Inclusion only supported for Optimism, Base, Linea and their Sepolia variants"
+ );
+ }If disallowing l1Inclusion for Eth mainnet is really intended, it should then also be enforced in the guest program, which is not the case currently, and any user can provide alternative inputs to prove for Eth mainnet with l1Inclusion, as shown above (set parameter
env_input_eth_for_l1_inclusionto a dummy non-empty value).
In that case, one can change the contracts to accept proofs which do not have l1Inclusion in case chainId is the one from Eth mainnet:
function _verifyProof(bytes calldata journalData, bytes calldata seal) internal view {
require(journalData.length > 0, mErc20Host_JournalNotValid());
// Decode the dynamic array of journals.
bytes[] memory journals = _decodeJournals(journalData);
// Check the L1Inclusion flag for each journal.
bool isSequencer = _isAllowedFor(msg.sender, _getProofForwarderRole())
|| _isAllowedFor(msg.sender, _getBatchProofForwarderRole());
if (!isSequencer) {
for (uint256 i = 0; i < journals.length; i++) {
- (,,,,,, bool L1Inclusion) = mTokenProofDecoderLib.decodeJournal(journals[i]);
- if (!L1Inclusion) {
+ (,,,,uint chainId,, bool L1Inclusion) = mTokenProofDecoderLib.decodeJournal(journals[i]);
+ if (!L1Inclusion && !(chainId == ETHEREUM_MAINNET)) {
revert mErc20Host_L1InclusionRequired();
}
}
}
// verify it using the IZkVerifier contract
verifier.verifyInput(journalData, seal);
}Barnadrot
Acknowledged:
We wont fix as the intended/crucial and required use-case for self-sequencing is permissionless exit which requires proving Linea state.
The fix is extensive and we have further proof upgrades required by external circumstances, therefore this is going to be remedied in a future version where self-sequencing latency and centralized sequencer latency is closer together.
Source: #304
0xmechanic, 10ap17, Angry_Mustache_Man, Cybrid, ExtraCaterpillar, PeterSR, ZanyBonzy, bube, bulgari, dan__vinci, elolpuer, farismaulana, oxwhite
The sendMsg in EverclearBridge contract only approves the transfer amount (not including the fee) for the FeeAdapter when calling newIntent. However, the FeeAdapter attempts to pull both the amount and the fee from the bridge contract. This results in a revert due to insufficient allowance causing cross-chain rebalancing to fail.
The root case stem from a mismatch between the approved amount and the actual amount required by the FeeAdaptor.
The sendMsg() in everclearBridge.sol is as follows:
function sendMsg(
uint256 _extractedAmount,
address _market,
uint32 _dstChainId,
address _token,
bytes memory _message,
bytes memory // unused
) external payable onlyRebalancer {
IntentParams memory params = _decodeIntent(_message);
require(params.inputAsset == _token, Everclear_TokenMismatch());
require(_extractedAmount >= params.amount, BaseBridge_AmountMismatch());
uint256 destinationsLength = params.destinations.length;
require(destinationsLength > 0, Everclear_DestinationsLengthMismatch());
bool found;
for (uint256 i; i < destinationsLength; ++i) {
if (params.destinations[i] == _dstChainId) {
found = true;
break;
}
}
require(found, Everclear_DestinationNotValid());
if (_extractedAmount > params.amount) {
uint256 toReturn = _extractedAmount - params.amount;
@> IERC20(_token).safeTransfer(_market, toReturn);//@audit-info the excess is sent back to the market
emit RebalancingReturnedToMarket(_market, toReturn, _extractedAmount);
}
@> SafeApprove.safeApprove(params.inputAsset, address(everclearFeeAdapter), params.amount);//@audit approve is done for amount
(bytes32 id,) = everclearFeeAdapter.newIntent(
params.destinations,
params.receiver,
params.inputAsset,
params.outputAsset,
params.amount,
params.maxFee,
params.ttl,
params.data,
params.feeParams
);
emit MsgSent(_dstChainId, _market, params.amount, id);
}The newIntent method in feeAdaptor contract is defined as:
/// @inheritdoc IFeeAdapter
function newIntent(
uint32[] memory _destinations,
bytes32 _receiver,
address _inputAsset,
bytes32 _outputAsset,
uint256 _amount,
uint24 _maxFee,
uint48 _ttl,
bytes calldata _data,
IFeeAdapter.FeeParams calldata _feeParams
) external payable returns (bytes32 _intentId, IEverclear.Intent memory _intent) {
// Transfer from caller
@> _pullTokens(msg.sender, _inputAsset, _amount + _feeParams.fee);//@audit amount+fee is pulled from bridge contract
// Create intent
(_intentId, _intent) =
_newIntent(_destinations, _receiver, _inputAsset, _outputAsset, _amount, _maxFee, _ttl, _data, _feeParams);
}As shown, _amount + _feeParams.fee is pulled from bridge contract, which will result in revert as the approve was done only for the specified amount.
There is an imbalance and sendMsg is invoked in the rebalancer contract.
Market B requires rebalancing Assets are available in Market A
- Market A on X chain has 12,000 USDC extractable value.
- Market B on Y chain needs 10,000 USDC.
- The
sendMsg()function is invoked in theRebalancercontract with_amount = 12,000 USDC. - The 12,000 USDC is sent to the
EverclearBridgecontract. Since the extracted amount is greater than the amount specified in the message, the excess 2,000 USDC is sent back to Market A. - The
FeeAdaptorcontract is approved for only 10,000 USDC. - The
newIntent()function is called on theFeeAdaptor. This function attempts to pull10,000 USDC + fee, but it reverts due to insufficient approval. As result, the rebalancing will not succeed.
Additinal Notes: According to the FeeAdaptor and Spoke contract logic in everclear platform, the fee is not deducted from the specified transfer amount but instead added on top during token transfer. Therefore, the fee cannot be zero, and any under-approval results in failure. If the fee were instead deducted from the approved amount, Market B would receive less than 10,000 USDC, potentially leading to issues like insufficient liquidity.
All cross-chain rebalancing operations using EverclearBridge will fail.
See the function flow and contract state change given above
Update the allowance logic in EverclearBridge to approve enough amount for the FeeAdapter.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: https://github.com/malda-protocol/malda-lending/pull/114/files
CergyK
Fix looks good
Source: #317
0xsai, yoooo
In the EverclearBridge which is used by rebalancer to send cross-chain intents using Everclear does not enforce that the maxFee and ttl parameters are set to 0, as required for the netting pathway. The Everclear documentation specifies that rebalancers must use the netting pathway, where maxFee == 0 (no solver fees) and ttl == 0 (immediate processing by the hub). However, the contract passes these parameters to everclearFeeAdapter.newIntent without validation, allowing non-zero values that could misroute intents to the unsupported solver pathway.
The sendMsg function in EverclearBridge.sol lacks validation checks to ensure params.maxFee == 0 and params.ttl == 0 before calling everclearFeeAdapter.newIntent as specified in the everclear docs..
function sendMsg(
uint256 _extractedAmount,
address _market,
uint32 _dstChainId,
address _token,
bytes memory _message,
bytes memory // unused
) external payable onlyRebalancer {
IntentParams memory params = _decodeIntent(_message);
require(params.inputAsset == _token, Everclear_TokenMismatch());
require(_extractedAmount >= params.amount, BaseBridge_AmountMismatch());
uint256 destinationsLength = params.destinations.length;
require(destinationsLength > 0, Everclear_DestinationsLengthMismatch());
bool found;
for (uint256 i; i < destinationsLength; ++i) {
if (params.destinations[i] == _dstChainId) {
found = true;
break;
}
}
require(found, Everclear_DestinationNotValid());
if (_extractedAmount > params.amount) {
uint256 toReturn = _extractedAmount - params.amount;
IERC20(_token).safeTransfer(_market, toReturn);
emit RebalancingReturnedToMarket(_market, toReturn, _extractedAmount);
}
SafeApprove.safeApprove(params.inputAsset, address(everclearFeeAdapter), params.amount);
(bytes32 id,) = everclearFeeAdapter.newIntent(
params.destinations,
params.receiver,
params.inputAsset,
params.outputAsset,
params.amount,
params.maxFee,
params.ttl,
params.data,
params.feeParams
);
emit MsgSent(_dstChainId, _market, params.amount, id);
}- Already does not validates
maxFeeandttl
- both
maxFeeandttl being none zero
- A rebalancer calls the
sendMsgto create a cross-chain intent with both themaxFeeandttlbeing none zero - The _decodeIntent function decodes the message into IntentParams, extracting maxFee (type uint24) and ttl (type uint48).
- The FeeAdapter may interpret non-zero maxFee or ttl as a solver pathway intent, misrouting the intent to solvers instead of the netting system.
- Since the solver pathway is not supported at launch, the intent may fail to settle, or, if solvers are active, it may incur unexpected fees or settle on a suboptimal chain.
- A potential DOS
- Non-zero maxFee or ttl could route intents to the solver pathway, which is unsupported, leading to settlement failures or delays that disrupt liquidity rebalancing across supported chains (e.g., Ethereum, Base, Linea, Optimism, Unichain, Arbitrum).
- may lead to loss of funds.
- when maxFee is not enforced as 0 the everclear netting pathway will not be used and protocol will not be able to cut their cost up to 10x
.
enforce both fields are validated..
function sendMsg(
uint256 _extractedAmount,
address _market,
uint32 _dstChainId,
address _token,
bytes memory _message,
bytes memory // unused
) external payable onlyRebalancer {
IntentParams memory params = _decodeIntent(_message);
require(params.inputAsset == _token, Everclear_TokenMismatch());
require(_extractedAmount >= params.amount, BaseBridge_AmountMismatch());
// Enforce netting pathway requirements
require(params.maxFee == 0, Everclear_InvalidMaxFee());
require(params.ttl == 0, Everclear_InvalidTtl());
uint256 destinationsLength = params.destinations.length;
require(destinationsLength > 0, Everclear_DestinationsLengthMismatch());
bool found;
for (uint256 i; i < destinationsLength; ++i) {
if (params.destinations[i] == _dstChainId) {
found = true;
break;
}
}
require(found, Everclear_DestinationNotValid());
if (_extractedAmount > params.amount) {
uint256 toReturn = _extractedAmount - params.amount;
IERC20(_token).safeTransfer(_market, toReturn);
emit RebalancingReturnedToMarket(_market, toReturn, _extractedAmount);
}
SafeApprove.safeApprove(params.inputAsset, address(everclearFeeAdapter), params.amount);
(bytes32 id,) = everclearFeeAdapter.newIntent(
params.destinations,
params.receiver,
params.inputAsset,
params.outputAsset,
params.amount,
params.maxFee, // Now guaranteed to be 0
params.ttl, // Now guaranteed to be 0
params.data,
params.feeParams
);
emit MsgSent(_dstChainId, _market, params.amount, id);
}sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: malda-protocol/malda-lending#143
Issue M-11: There is no endpoint for triggering liquidateExternal from extension chain to be executed by proof forwarder
Source: #370
ExtraCaterpillar, PeterSR, cergyk, mussucal
mErcHost has a liquidateExternal endpoint for using liquidity coming from other chains to perform a liquidation, but there's no equivalent endpoint in mTokenGateway to trigger the liquidation on host chain.
Please note that
supplyOnHostcan be used to increaseaccAmountIn, but there's no way to specify the borrower which should be liquidated.
function supplyOnHost(uint256 amount, address receiver, bytes4 lineaSelector)
external
payable
override
notPaused(OperationType.AmountIn)
onlyAllowedUser(msg.sender)
ifNotBlacklisted(msg.sender)
ifNotBlacklisted(receiver)
{
// checks
require(amount > 0, mTokenGateway_AmountNotValid());
require(msg.value >= gasFee, mTokenGateway_NotEnoughGasFee());
IERC20(underlying).safeTransferFrom(msg.sender, address(this), amount);
// effects
accAmountIn[receiver] += amount;
emit mTokenGateway_Supplied(
msg.sender,
receiver,
accAmountIn[receiver],
accAmountOut[receiver],
amount,
uint32(block.chainid),
LINEA_CHAIN_ID,
//@audit can specify selector, but not borrower to liquidate on host chain
>> lineaSelector
);
}liquidateExternal cannot be called from extension chains to be executed by proof forwarder. Since as we have seen in some issues such as l1Inclusion issue, it is not possible to make some actions permissionlessly (when extension chain is Eth mainnet), this makes impossible to call liquidateExternal from Eth mainnet.
Either add a new endpoint liquidateOnHost, or some extra data for supplyOnHost to be able to specify a borrower to liquidate and pass liquidateExternal as lineaSelector.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: https://github.com/malda-protocol/malda-lending/pull/106/files
CergyK
Fix looks good. The liquidate method has been added to mTokenGateway and the selector is now handled in BatchSubmitter
Source: #686
befree3x
The REBALANCER_EOA role, which is considered semi-trusted, can drain funds from any market by specifying an arbitrarily high maxFee when initiating a bridge transfer through EverclearBridge.sol. This violates the trust assumption that the Rebalancer "cannot transfer user funds" and can only perform DDoS-style attacks, as it allows for a slow but steady extraction of value from the protocol's liquidity pools.
The Rebalancer::sendMsg function allows the REBALANCER_EOA to pass an unchecked _message blob to the selected bridge contract. The EverclearBridge.sendMsg function decodes this message to extract bridging parameters, including a maxFee. However, the bridge contract fails to validate this maxFee against any protocol-defined limit, instead passing it directly to the external everclearFeeAdapter.newIntent call. A malicious REBALANCER_EOA can therefore set this fee to an extreme value, causing the protocol to lose most of the bridged amount to fees.
- A market (e.g.,
mWethHost) has liquidity. - The
EverclearBridgeis whitelisted in the Rebalancer.
The REBALANCER_EOA private key is compromised or its operator acts maliciously.
A malicious actor controlling the REBALANCER_EOA decides to drain funds from the mWethHost market.
- The actor calls
Rebalancer.sendMsg, targeting theEverclearBridgeandmWethHost. - It provides a valid
_amountto extract from the market, for example,10 WETH. - It crafts the
_messageparameter. Inside this message, which is decoded byEverclearBridge, it sets the amount to be bridged to 10 WETH but sets themaxFeeparameter to an extremely high value, such as9.9 WETH. - The
Rebalancercontract extracts10 WETHfrommWethHostand callsEverclearBridge.sendMsg. EverclearBridgedecodes the message but performs no validation on themaxFee.- It calls
everclearFeeAdapter.newIntent, passing along the malicious maxFee of9.9 WETH. - The external
Everclearprotocol executes the bridge transfer. It sends10 WETHbut is authorized to take up to9.9 WETHas a fee, which is lost from the protocol. Only 0.1 WETH (or less) arrives at the destination market. - The attacker can repeat this process, draining a substantial portion of the market's liquidity over time through exorbitant fees.
Rebalancer can cause permanent loss of protocol funds. This attack directly drains the liquidity provided by users from the market contracts. While the REBALANCER_EOA does not receive the funds directly, their action causes value to be extracted from the protocol and paid to third-party bridge relayers.
No response
The protocol should not blindly trust the maxFee parameter provided by the semi-trusted REBALANCER_EOA. A centrally-controlled, maximum allowable fee should be enforced.
- The
GUARDIAN_BRIDGErole (a more trusted admin) should set a maximum fee limit (e.g., as a percentage or basis points) on a per-token, per-chain basis within theRebalanceror bridge contracts. - The
EverclearBridge.sendMsgfunction must validate that themaxFeeparameter decoded from the message does not exceed the configured maximum fee limit for that specific bridging route.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: malda-protocol/malda-lending#139
CergyK
Fix looks good
Source: #741
0xDemon, 0xKann, 0xlucky, 7, BADROBINX, Cybrid, ExtraCaterpillar, IvanFitro, Richard796, TECHFUND-inc, WillyCode20, Yaneca_b, ZanyBonzy, Ziusz, bube, dany.armstrong90, dimah7, elolpuer, farismaulana, gh0xt, harry, molaratai, mussucal, sakibcy, teoslaf1, weblogicctf
WrapAndSupply::wrapAndSupplyOnExtensionMarket first calls deposit to the underlying token contract implementation via the _wrap() function , which takes all the msg.value provided in the transaction, and turns it into the minted tokens. As a result, the subsequent call to supplyOnHost can potentialy always lead to revert if gasFee is set, since all of the msg.value is used AND since all of this happens in single transactoin. There is no amount left for the gasFee. This is a problem because it disrupts the implementation of the crucial functionality for setting gas fees. Transactions passing through the WrapAndSupply service for extension markets are only successful when gasFee = 0.
The functionality that wraps a native coint into its wrapped version uses all of the msg.value provided by the user without taking into account the possible requirement of additional value for gas fees.
- WrappedNative implementation and mToken market with
underlyingset to it have to exist - Onwer needs to set gasFee to the
mTokenGatewaycontract.
- User has to call the wrap and supply utility provided by the protocol.
.
Breaks core contract functionality
- Used test file: "test/unit/mTokenGateway/mTokenGateway_supplyOnHost.sol"
- added optional console logs for clarity
import {console} from "forge-std/console.sol";
contract mTokenGateway_supplyOnHost is mToken_Unit_Shared {
... function test_WrapAndSupply() external {
// set gas fee
vm.startPrank(address(mWethExtension.owner()));
mWethExtension.setGasFee(1 wei);
// console.log("gasFee: ", mWethExtension.gasFee());
vm.stopPrank();
// wrap and supply
vm.startPrank(address(this));
// 10000000000000000000 10e18
vm.deal(address(this), SMALL);
// console.log(address(this).balance);
WrapAndSupply wrapAndSupply = new WrapAndSupply(address(weth));
vm.label(address(wrapAndSupply), "WrapAndSupply Helper");
uint256 accAmountInBefore = mWethExtension.accAmountIn(address(this));
// console.log(accAmountInBefore);
wrapAndSupply.wrapAndSupplyOnExtensionMarket{value: SMALL}(
address(mWethExtension), address(this), mTokenGateway_supplyOnHost.test_RevertWhen_AmountIs0.selector
);
vm.stopPrank();
}The test reverts with the "mTokenGateway_NotEnoughGasFee()" error as expected
[FAIL: mTokenGateway_NotEnoughGasFee()] test_WrapAndSupply() (gas: 565393)Possible solution is to fetch the gasFee price before calling the wrap and supply function, so he can provide enough amount to pass. And then within the _wrap() calculate the new amount to wrap.
- add view function to the gateway interface
function gasFee() external view returns (uint256);- recalculate mint amount
uint256 _gasFee = ImTokenGateway(wrappedNative).gasFee();
uint256 amount = msg.value - _gasFee;sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: malda-protocol/malda-lending#111
CergyK
Changes requested on the fix
Barnadrot
changes have been added
CergyK
Fix looks good
Issue M-14: MixedPriceOracleV4.sol :: getUnderlyingPrice()/getPirce() will not work for some tokens because API3 and EO oracles return prices using different decimals, causing DOS scenario.
Source: #945
0xEkko, 0xfocusNode, 10ap17, Cybrid, HeckerTrieuTien, IvanFitro, KiroBrejka, PeterSR, SafetyBytes, ZanyBonzy, Ziusz, axelot, dan__vinci, elolpuer, pashap9990
getUnderlyingPrice()/getPrice() are used to obtain the price of the assets in USD. It does this by querying two different oracles, in this case, API3 and EO oracles. The problem is that these oracles return prices with different decimals, causing the function to always end up using the EO oracle’s price.
getUnderlyingPrice() calls _getPriceUSD(), which in turn calls _getLatestPrice(), implemented as follows:
function _getLatestPrice(string memory symbol, PriceConfig memory config)
internal
view
returns (uint256, uint256)
{
if (config.api3Feed == address(0) || config.eOracleFeed == address(0)) revert MixedPriceOracle_MissingFeed();
//get both prices
@> (, int256 apiV3Price,, uint256 apiV3UpdatedAt,) = IDefaultAdapter(config.api3Feed).latestRoundData();
@> (, int256 eOraclePrice,, uint256 eOracleUpdatedAt,) = IDefaultAdapter(config.eOracleFeed).latestRoundData();
// check if ApiV3 price is up to date
uint256 _staleness = _getStaleness(symbol);
bool apiV3Fresh = block.timestamp - apiV3UpdatedAt <= _staleness;
// check delta
@> uint256 delta = _absDiff(apiV3Price, eOraclePrice);
uint256 deltaBps = (delta * PRICE_DELTA_EXP) / uint256(eOraclePrice < 0 ? -eOraclePrice : eOraclePrice);
uint256 deltaSymbol = deltaPerSymbol[symbol];
if (deltaSymbol == 0) {
deltaSymbol = maxPriceDelta;
}
uint256 decimals;
uint256 uPrice;
@> if (!apiV3Fresh || deltaBps > deltaSymbol) {
require(block.timestamp - eOracleUpdatedAt < _staleness, MixedPriceOracle_eOracleStalePrice());
decimals = IDefaultAdapter(config.eOracleFeed).decimals();
uPrice = uint256(eOraclePrice);
} else {
require(block.timestamp - apiV3UpdatedAt < _staleness, MixedPriceOracle_ApiV3StalePrice());
decimals = IDefaultAdapter(config.api3Feed).decimals();
uPrice = uint256(apiV3Price);
}
return (uPrice, decimals);
}As you can see, it obtains the price from both oracles and calculates the absolute difference using _absDiff():
function _absDiff(int256 a, int256 b) internal pure returns (uint256) {
return uint256(a >= b ? a - b : b - a);
}The problem is that the oracles do not return prices with the same decimals. According to the documentation, API3 always returns prices with 18 decimals, while the EO oracle almost always returns prices with 8 decimals.
As a result, _absDiff() ends up calculating the difference between an 18-decimal price and an 8-decimal price, producing an incorrect delta. This delta will be disproportionately large—essentially because 18 decimals - 8 decimals ≈ 18 decimals—causing the first if condition to be triggered almost every time (because _delta is capped by PRICE_DELTA_EXP = 1e5). This leads to always selecting the EO oracle’s price, effectively making the dual-oracle setup useless.
Always entering the first if is problematic because if apiV3Fresh = true (meaning the API3 price is not stale) but the EO oracle price is stale, the transaction will still revert with MixedPriceOracle_eOracleStalePrice(). This happens even when the prices themselves are correct, simply because deltaBps > deltaSymbol is true due to the decimal mismatch. As a result, it creates a DOS for obtaining the price, when in reality the price could be retrieved using API3 orcale if the decimals from both oracles were adjusted correctly.
None.
API decimals > EO decimals or vice versa. API price is fresh, and EO price is stale.
None.
ThThe price obtained using getUnderlyingPrice() will always come from the EO oracle. If the EO price is stale while the API3 price is fresh, the price cannot be obtained, creating a denial-of-service (DoS) scenario.
To illustrate the problem, let’s use the wBTC / USD pair on the Linea chain, which is in scope for the contest. In the EO oracle, the price is returned with 8 decimals, while in API3 it’s returned with 18 decimals.
getUnderlyingPrice()is called for an mToken whose underlying asset is wBTC.- The EO oracle price is
120000000000(8 decimals), and the API3 oracle price is120000000000000000000000(18 decimals). The API3 price is fresh, while the EO oracle price is stale. _absDiff()is calculated as120000000000000000000000 - 120000000000 ≈ 120000000000000000000000, producing a very large delta.- Because
deltaBpsis much greater thandeltaSymbol, the firstifcondition is triggered, selecting the EO oracle’s price. However, this reverts because the EO price is stale.
If deltaBps were calculated correctly, the else branch would be taken, and the fresh API3 price would be used. In the current implementation, this creates a denial of service (DoS) scenario where the price cannot be obtained.
To solve the problem, normalize the decimals from both oracles before calculating _absDiff(), ensuring the deltaBps is computed correctly.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: malda-protocol/malda-lending#134
CergyK
Fix looks good, as already mentioned in #1305
Source: #1309
0xapple, 10ap17, ExtraCaterpillar, SafetyBytes, ZeroTrust, cergyk
If the Across bridging process fails, funds intended for bridging become locked in the Rebalancer contract, making them inaccessible.
When the Across bridge calls the depositV3Now function, the depositor is set as the Rebalancer. According to the Across documentation, if a deposit expires or a fill fails, funds are refunded to the depositor on the origin chain. When the bridging process using Across fails, the refunded assets are returned to the Rebalancer, making them completely locked and inaccessible. From Across docs:
In cases where a slow fill can't or does not happen and a relayer does not fill the intent, the intent expires. Like a slow fill, this expiry must be optimistically verified, which takes a few hours. Once this verification is done, the user is then refunded their money on the origin chain.
As seen bellow, Rebalancer is set as depositor:
function _depositV3Now(bytes memory _message, address _token, uint32 _dstChainId, address _market) private {
DecodedMessage memory msgData = _decodeMessage(_message);
// approve and send with Across
SafeApprove.safeApprove(_token, address(acrossSpokePool), msgData.inputAmount);
IAcrossSpokePoolV3(acrossSpokePool).depositV3Now( // no need for `msg.value`; fee is taken from amount
msg.sender, //depositor
address(this), //recipient
_token,
address(0), //outputToken is automatically resolved to the same token on destination
msgData.inputAmount,
msgData.outputAmount, //outputAmount should be set as the inputAmount - relay fees; use Across API
uint256(_dstChainId),
msgData.relayer, //exclusiveRelayer
msgData.deadline, //fillDeadline
msgData.exclusivityDeadline, //can use Across API/suggested-fees or 0 to disable
abi.encode(_market)
);
}/
/
- The Rebalancer initiates the bridging process using AcrossBridge.
- The depositV3Now function is called with the Rebalancer set as the depositor.
- The bridging process fails.
- Funds are refunded to the Rebalancer(depositor) on the origin chain.
- The funds are now locked, as there is no mechanism for the Rebalancer to use them or return them to the market.
Funds become completely locked inside the Rebalancer contract and are effectively lost.
No response
Consider adding helper function in Rebalancer contract, that could rescue returned tokens to the intended market.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: malda-protocol/malda-lending#126
CergyK
Fix looks good
Barnadrot
Added the suggested changes, good to merge now?
CergyK
Added the suggested changes, good to merge now?
Yes good to merge
Source: #1477
10ap17, holtzzx
/
Across and Everclear, don't support all of the assets that are listed as supported. That will result in inability to rebalance markets for that specific tokens
/
/
No specific attack path, since the bridge don't support these assets
Rebalancing will be impossible for those markets.
No response
No response
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: malda-protocol/malda-lending#100