Feat/2026 q2 contracts scope extended - #246
Conversation
There was a problem hiding this comment.
Code Review
This pull request upgrades the Midas protocol contracts to Solidity 0.8.34, introducing a global pause manager, a timelock manager, and updated access control, while refactoring the deposit and redemption vaults. The code review identified several critical issues: a decoding bug in MidasTimelockManager due to unmasked bytes4 selector loading, a potential fund-locking issue in RedemptionVault caused by unconditional minimum amount checks during request approvals, and a block on mToken clawbacks when a victim's remaining balance falls below the minimum threshold. Additionally, inconsistent recipient tracking in DepositVault could allow users to bypass first-deposit limits.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| bytes4 selector; | ||
|
|
||
| // getting the selector of custom error | ||
| assembly { | ||
| selector := mload(add(err, 32)) | ||
| } | ||
|
|
||
| // checking if the error is a RolePreflightSucceeded error | ||
| require( | ||
| selector == RolePreflightSucceeded.selector, | ||
| InvalidPreflightError(err) | ||
| ); |
There was a problem hiding this comment.
In Solidity, bytes4 variables are left-aligned and right-padded with zeros. When loading the selector using mload(add(err, 32)), the lower 28 bytes of the loaded 32-byte word are dirty because they contain the beginning of the role argument. The Solidity compiler assumes that bytes4 variables are clean and performs a direct 32-byte comparison (eq) for selector == RolePreflightSucceeded.selector. Because of the dirty right-padding, this comparison will always evaluate to false, causing the preflight decoding to always revert with InvalidPreflightError. To fix this, you must mask the loaded value to clear the lower 28 bytes.
bytes4 selector;
// getting the selector of custom error
assembly {
selector := and(mload(add(err, 32)), 0xffffffff00000000000000000000000000000000000000000000000000000000)
}
// checking if the error is a RolePreflightSucceeded error
require(
selector == RolePreflightSucceeded.selector,
InvalidPreflightError(err)
);
| { | ||
| _requireTokenExists(tokenOut); | ||
|
|
||
| _validateMTokenAmount(user, amountMTokenIn); |
There was a problem hiding this comment.
In _calcAndValidateRedeem, _validateMTokenAmount is called unconditionally. During _approveRequest (asynchronous redemption approval), _calcAndValidateRedeem is called with user = request.recipient. If the recipient is not exempt from the minimum amount (isFreeFromMinAmount is false) and the redemption amount is below minAmount, _validateMTokenAmount will revert. Since the request was already created and the user's mTokens were already transferred to requestRedeemer, this revert prevents the admin from ever approving or rejecting the request, permanently locking the user's funds in the contract. To fix this, the minimum amount check should only be performed when isInstant is true.
if (isInstant) {
_validateMTokenAmount(user, amountMTokenIn);
} else {
require(amountMTokenIn > 0, InvalidAmount());
}
| function _validateMinBalance(address from, address to) private view { | ||
| if (!isMinHoldingBalanceEnforced) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
If clawback is executed, it transfers tokens from the victim to clawbackReceiver. If this transfer leaves the victim with a non-zero balance below 1 ether, _validateUserMinBalance will revert in _afterTokenTransfer, blocking the clawback. To prevent clawback from being blocked by the victim's remaining balance, _validateMinBalance should be bypassed if _inClawback is true.
function _validateMinBalance(address from, address to) private view {
if (!isMinHoldingBalanceEnforced || _inClawback) {
return;
}
| result = _calcAndValidateDeposit(user, tokenIn, amountToken, true); | ||
|
|
||
| require( | ||
| result.mintAmount >= minReceiveAmount, | ||
| "DV: minReceiveAmount > actual" | ||
| ); | ||
| _requireSlippageNotExceeded(result.mintAmount, minReceiveAmount); | ||
|
|
||
| totalMinted[user] += result.mintAmount; |
There was a problem hiding this comment.
In _depositInstant, totalMinted is tracked and checked against msg.sender (via user). However, in _approveRequest (request deposit approval), totalMinted is tracked against request.recipient. This inconsistency allows users to bypass the first deposit limit by depositing through a router or a helper contract, and also subjects legitimate users to the limit multiple times if they deposit to different recipients. To fix this, the first deposit check and totalMinted tracking should consistently use recipient instead of msg.sender.
result = _calcAndValidateDeposit(recipient, tokenIn, amountToken, true);
_requireSlippageNotExceeded(result.mintAmount, minReceiveAmount);
totalMinted[recipient] += result.mintAmount;
| CalcAndValidateDepositResult | ||
| memory calcResult = _calcAndValidateDeposit( | ||
| user, | ||
| tokenIn, | ||
| amountToken, | ||
| false | ||
| ); |
There was a problem hiding this comment.
In _depositRequest, _calcAndValidateDeposit is called with user (which is msg.sender). To be consistent with the tracking of totalMinted for the recipient at approval time, the first deposit check should be performed against recipient instead of msg.sender.
CalcAndValidateDepositResult
memory calcResult = _calcAndValidateDeposit(
recipient,
tokenIn,
amountToken,
false
);
…-q2-contracts-scope-extended
No description provided.