Remove Redundant Assembly Masking in SafeTransferLib + Prevent Zero-Asset Mint in ERC4626#436
Open
timeless-hayoka wants to merge 1 commit into
Conversation
…sset Mint in ERC4626
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remove Redundant Assembly Masking in SafeTransferLib + Prevent Zero-Asset Mint in ERC4626
Removes redundant assembly masking based on Solidity ≥0.8.0 behavior, and fixes a zero-asset mint edge case in ERC4626 with test-backed validation.
1. Gas Optimization: Redundant Masking in
SafeTransferLibProblem
In
SafeTransferLib.sol, the inline assembly blocks manually mask thefromandtoaddress arguments usingand(address, 0xff...ff). This was a common defensive programming practice in Solidity<0.8.0to ensure dirty upper bits were cleared before execution.Root Cause & Justification
Under Solidity
>=0.8.0, address-typed variables passed from Solidity into inline assembly are already sanitized by the compiler, making additional masking redundant in this context. There is no external raw calldata bypass that can sneak dirty bits into these specific local variables before the Yul block executes.Solution
Removed the redundant
and(..., 0xff...)masks from the inline assembly.Impact
safeTransfer/safeApprovecall, and 10 gas persafeTransferFromcall. In DeFi protocols performing millions of transfers, this aggregate savings is significant.forge test --gas-report:testTransferWithStandardERC20() (gas: 64633)testTransferWithStandardERC20() (gas: 64627)No functional behavior changes; all existing tests pass unchanged.
2. Security Edge Case: Infinite Free Minting on Empty ERC4626 Vault
Problem
The
ERC4626.solimplementation does not validate thatassets != 0during themintsequence. While this is mathematically expected behavior becausepreviewMintrounds up, it introduces a critical vulnerability when a vault hastotalSupply > 0but suffers a 100% loss (totalAssets() == 0). This condition can occur after slashing events, oracle failures, or external asset loss.Root Cause
When
totalAssets() == 0buttotalSupply > 0,previewMint(shares)callsmulDivUp(totalAssets(), totalSupply).This evaluates to
(shares * 0) / totalSupplywhich is exactly0.The
mintfunction proceeds to transfer0assets from the caller, but still mints the requestedshares.An attacker can monitor a fully drained vault and mint an arbitrary amount of shares for zero assets, instantly diluting the pool so that if any assets are later recovered or donated back, the attacker can capture any subsequently deposited or recovered assets.
Solution
Added a strict non-zero check in
mint, symmetrical todeposit:require(assets != 0, "ZERO_ASSETS");This mirrors the invariant already enforced in deposit (
shares != 0), ensuring consistent behavior across entry points.Impact & Proof
Protects ERC4626 integrations from being permanently bricked or stolen from during recovery scenarios following a slashing event.
Test Validation (
testRevertMintZeroAssetsWhenVaultDrained):Test fails on current main branch and passes with this fix.
"ZERO_ASSETS".