Organization
- @uniswap
Engagement Type
Cantina Reviews
Period
-
Repositories
Findings
High Risk
2 findings
1 fixed
1 acknowledged
Low Risk
4 findings
2 fixed
2 acknowledged
Informational
8 findings
7 fixed
1 acknowledged
High Risk2 findings
execute calls can be front-run
Description
The function:
function execute(SignedBatchedCall memory signedBatchedCall, bytes memory wrappedSignature) public payable
implemented in the
MinimalDelegation
contract is publicly callable, enabling any external address to invoke it if a valid signature is provided. This implementation allows anyone to front-run anyexecute
call as the code simply checks the signature and does not confirm the identity of the caller. Since there is no field for the intended executor address in the signed digest, any party that obtains the signature can submit it first.A very detrimental scenario could be a malicious user supplying no Ether (e.g.,
msg.value == 0
) in a front-runexecute
call that was supposed to use it, potentially forcing part of the batched calls to revert. IfsignedBatchedCall.shouldRevert = false
, the attacker can easily break the intended call flow. Meanwhile, the legitimate user’s subsequent call will revert because the same signature and nonce have already been consumed.Recommendation
Incorporate an intended executor or caller address into the signature’s domain data to ensure only the authorized caller can use that signature. Specifically:
- Introduce a field (e.g.
executor
) in the struct that is hashed and checked in the EIP-712 domain. - If
executor
is not zero, verify at runtimerequire(msg.sender == executor);
if zero, fallback to the existing “anyone can execute” logic if truly intended.
Uniswap: Fixed in PR #150.
Cantina: Fix verified.
- Introduce a field (e.g.
execute calls can be forced to fail with an out of gas error
State
- Acknowledged
Severity
- Severity: High
Submitted by
r0bert
Description
In the
execute(SignedBatchedCall memory signedBatchedCall, bytes memory wrappedSignature) public payable
flow, a malicious user can specify a gas limit for the overall transaction that is large enough for the “high-level” portion of theexecute
call to succeed but leaves insufficient gas for the low-level call performed in_dispatch
→_execute
, where:(success, output) = to.call{value: _call.value}(_call.data);
Due to the EIP-150 “63/64 gas” rule, only 63/64 of the remaining gas is forwarded to a subcall. If the subcall fails for insufficient gas and
signedBatchedCall.shouldRevert == false
, the entire batch may partially complete with no revert, thus forcing the intended function call to fail. As a result, the user’s signed batch is sabotaged by the attacker controlling the available gas, while the high-level transaction still succeeds consuming the signature's nonce.Recommendation
Consider following OpenZeppelin’s MinimalForwarder.sol implementation.
Another alternative is storing a minimum intended gas limit in the signed data: if the
execute
call is given a gas limit below that limit, revert.Uniswap: Acknowledged. we lean towards not fixing this because:
- Introducing the
executor
field inSignedBatchedCall
allows for users to require a specific relayer for their actions which mitigates the severity of this attack - We believe that it would be relatively easy to detect when this is happening, and that the impact would be relatively small for users (if you are sending a batch which can partially revert, you should be aware of all of the possibilities)
Low Risk4 findings
Potential double-counting allowance risk
State
- Acknowledged
Severity
- Severity: Low
Submitted by
r0bert
Description
The
MinimalDelegation
wallet supports two forms of native allowances:- Persistent approvals: stored in regular contract storage via
approveNative
andtransferFromNative
. - Transient approvals: granted by
approveNativeTransient
and used bytransferFromNativeTransient
, which reset each transaction.
Because the code separately tracks ephemeral and persistent allowances, a user’s total effective approval can unintentionally stack. For example, if the user or the contract sets a persistent allowance of 1000 and then separately grants a transient allowance of 1000, the spender might see an aggregate of 2000. This could exceed the user’s intended limit and lead to accidental over-spending.
Recommendation
Consider disallowing calling
approveNativeTransient
if the persistent allowance is already non-zero for thatspender
. Another option is simply informing the users that transient and persistent allowances are cumulative and stating this in the user wallet documentation.Uniswap: Acknowledged. We lean towards not fixing this because:
- If there exists a persistent allowance AND a transient one, we don't believe that it is a double spend because there exists two intentionally set and distinct allowances for the spender
- We expect any reasonably written spender contract to be explicit about which allowance it is requesting (persistent or transient), and to only use those amounts when needed
off-by-one issue in isExpired()
Description
When the protocol checks if the setting is expired, it considers
expiration == block.timestamp
to be invalid.function isExpired(Settings settings) internal view returns (bool _isExpired, uint40 _expiration) { uint40 _exp = expiration(settings); if (_exp == 0) return (false, 0); return (_exp <= block.timestamp, _exp); }
While in
validateUserOp()
,expiration
is used asvalidUntil
invalidationData
. And in EntryPoint,validUntil == block.timestamp
is considered valid.function _getValidationData( uint256 validationData ) internal virtual view returns (address aggregator, bool outOfTimeRange) { if (validationData == 0) { return (address(0), false); } ValidationData memory data = _parseValidationData(validationData); // solhint-disable-next-line not-rely-on-time outOfTimeRange = block.timestamp > data.validUntil || block.timestamp <= data.validAfter; aggregator = data.aggregator; }
Recommendation
It is recommended to change
isExpired()
as follows to treatexpiration == block.timestamp
as valid.function isExpired(Settings settings) internal view returns (bool _isExpired, uint40 _expiration) { uint40 _exp = expiration(settings); if (_exp == 0) return (false, 0);- return (_exp <= block.timestamp, _exp);+ return (_exp < block.timestamp, _exp); }
Uniswap: Fixed in PR #147.
Cantina: Fix verified.
Potential privilege escalation in nonce management
State
- Acknowledged
Severity
- Severity: Low
Submitted by
cccz
Description
In
NonceManager
, allkeyHashes
use the same underlying nonce mapping.function _useNonce(uint256 nonce) internal { uint192 key = uint192(nonce >> 64); uint64 seq = uint64(nonce); if (!(nonceSequenceNumber[key]++ == seq)) { revert InvalidNonce(); } }
And by design, only
keyHashes
with admin privileges can invalidate any nonce byinvalidateNonce()
, butkeyHashes
without admin privileges can sign anynonceKey
to invalidate any nonce, which can be used in the front-run attack to invalidate execute calls from otherkeyHashes
.function invalidateNonce(uint256 newNonce) external onlyThis { uint192 key = uint192(newNonce >> 64); uint64 currentSeq = uint64(nonceSequenceNumber[key]); uint64 targetSeq = uint64(newNonce); if (targetSeq <= currentSeq) revert InvalidNonce(); // Limit the amount of nonces that can be invalidated in one transaction. unchecked { uint64 delta = targetSeq - currentSeq; if (delta > type(uint16).max) revert ExcessiveInvalidation(); } nonceSequenceNumber[key] = targetSeq; emit NonceInvalidated(newNonce); }
Recommendation
It is recommended to separate the nonces for different keyHashes.
- mapping(uint256 key => uint256 seq) nonceSequenceNumber;+ mapping(bytes32 => mapping(uint192 => uint256)) public nonceSequenceNumber;
Uniswap: Acknowledged. We lean towards not fixing this because:
- Admin keys can invalidate an entire
nonceKey
space, which is arguably a stronger DoS than a single value - There is plausible front-running risk from a malicious non admin key, but we believe that the user should simply remove that key from their account if detected
- It is nice to be able to enforce strict ordering of nonces between different keys, which would be impossible if we segmented nonces by
keyHash
_checkExpiry will revert if the signature is expired in validateUserOp
Description
Within the
validateUserOp
flow, the contract verifies the signature and then calls_checkExpiry(settings)
. If the key is expired,_checkExpiry(settings)
unconditionally reverts with aKeyExpired(expiry)
error. As a result, the entire operation fails with a revert rather than returningSIG_VALIDATION_FAILED
as stated in the EIP-4337 spec.Recommendation
Consider returning the signature failure code (e.g. SIG_VALIDATION_FAILED) if the key has expired instead of reverting in the
validateUserOp
function.On the other hand, in the
isValidSignature
function_checkExpiry(settings)
is also used. In this flow, consider settingisValid
tofalse
in case that the signature is expired.Uniswap: Fixed in PR #154.
Cantina: Fix verified.
Informational8 findings
ModeDecoder implements a subset of EIP‑7821
Description
In
ModeDecoder
andMinimalDelegation
, particularly in theexecute(bytes32 mode, bytes calldata executionData)
andsupportsExecutionMode(bytes32 mode)
functions, only recognizes two specific “batched” modes:bytes32 constant BATCHED_CALL = 0x0100000000000000000000000000000000000000000000000000000000000000;bytes32 constant BATCHED_CAN_REVERT_CALL = 0x0101000000000000000000000000000000000000000000000000000000000000;
and checks them via:
function isBatchedCall(bytes32 mode) internal pure returns (bool) { return mode == BATCHED_CALL || mode == BATCHED_CAN_REVERT_CALL;}
This approach diverges from the EIP‑7821 specification, which defines more granular modes. For example,
0x01000000000000000000...
or0x01000000000078210001...
for single-batch with optionalopData
and a multi-batch approach0x01000000000078210002....
Additionally,MinimalDelegation
does not parse any optionalopData
nor supports “batch of batches” recursion. Instead, it decodes only a singleCall[]
and toggles revert behavior on failure, ignoring the extended modes described in EIP‑7821. This design is not incorrect from a security perspective, but it means the contract remains incompatible with the standard’s optional features such as multi-level batches or specialized data for authentication.Recommendation
Provide clarity in code comments or docs that only two batch modes are supported. If partial coverage is the final design choice, specify the rationale. Otherwise, upgrade the contracts to align with the full EIP‑7821 spec.
Uniswap: Fixed in PR #149.
Cantina: Fix verified.
Incorrect comment
Description
In the
SettingsLib
library, the comment claims:/// The most significant 8 bits are reserved to specify if the key is an admin key or not./// 6 bytes | 1 byte | 5 bytes | 20 bytes
However, the code actually shifts by 200 bits (shr(200, settings)), using 56 bits (7 bytes) for the admin portion. This is inconsistent with the stated “8 bits” approach. Consequently, bits [200..255] become the “admin region,” not [248..255].
Recommendation
Consider updating the comment as shown below:
/// The most significant 7 bits are reserved to specify if the key is an admin key or not./// 6 bytes | 1 byte | 5 bytes | 20 bytes
Uniswap: Fixed in PR #156.
Cantina: Fix verified.
hookData is not included in the signature digest
Description
MinimalDelegation
exposeshookData
in calls tohandleAfterVerifySignature
andhandleAfterIsValidSignature
function, but does not incorporatehookData
into the EIP‑712 signature digest. Consequently, any external caller can supply arbitrary bytes inwrappedSignature
:(bytes32 keyHash, bytes memory signature, bytes memory hookData) = abi.decode(wrappedSignature, (bytes32, bytes, bytes));
and the contract then passes this untrusted
hookData
to the hook. If the hook logic expectshookData
to be genuine, an attacker can cause reverts or trigger unexpected behaviors. For example, the attacker can supply malicious input that is decoded incorrectly in the hook, forcing the transaction to revert. SincehookData
is not signed by the user’s private key, an attacker can alter it and the signature would still be valid.The final impact is really dependant on the
hook
's final implementation.Recommendation
Include
hookData
in the signed digest so if its updated by a malicious user the signature will become invalid.If the current implementation is kept, consider documenting that the
hookData
can be altered, so the hook should always be able to handle malicious data gracefully (e.g., ignoring it or only running safe logic).Uniswap: Fixed in PR #149.
Cantina: Fix verified.
MinimalDelegation EntryPoint compatibility
Description
After reviewing the latest
EntryPoint
versions, theMinimalDelegation
smart wallet can only be used with the v0.7.0 and v0.8.0 versions, being incompatible with the v0.6.0 as this version does not yet support theexecuteUserOp
operations. TheexecuteUserOp
support was introduced in the v0.7.0 version.EntryPoint version analysis
EntryPoint Version v0.6.0
Key Changes:
- New userOpHash algorithm
- Nonce managed by EntryPoint
- Prevent recursion of handleOps
Description:
- The new
userOpHash
algorithm introduces a unique hash for eachUserOperation
, enhancing security by preventing replay attacks. - Nonce managed by EntryPoint centralizes nonce management within the EntryPoint contract, simplifying the logic in account contracts and reducing the chance of errors.
- The system prevents recursion of
handleOps
, adding a safeguard against recursive calls during operation handling, which helps avoid exploits or infinite loops.
MinimalDelegation Compatibility: Incompatible as it does not support the
executeUserOp
operation and uses a differentUserOperation
struct.EntryPoint Version v0.7.0
Key Changes:
- Simulation functions removed from deployed EntryPoint
- Added
delegateAndRevert()
helper - Added ERC-165 "supportsInterface"
- Added a sample TokenPaymaster
- Added a 10% penalty charge for unused execution gas limit
Description:
- Simulation functions removed from the deployed contract reduces complexity and deployment costs, with simulation now handled off-chain by bundlers.
- The
delegateAndRevert()
helper assists bundlers in simulating transactions accurately, especially on networks without state override features. - ERC-165 "supportsInterface" enables interface detection, making the contract more interoperable with others.
- A sample
TokenPaymaster
provides a reference for paymasters that accept tokens instead of ETH for gas payments, encouraging broader adoption. - A 10% penalty for unused execution gas encourages precise gas estimation to improve network efficiency.
MinimalDelegation Compatibility: Fully compatible but lacks support for EIP-7702 authorizations.
EntryPoint Version v0.8.0
Key Changes:
- Native support for EIP-7702 authorizations
- Native support for ERC-712 based UserOperation hash and signatures
- Unused gas penalty adjustment (no penalty if below 40,000 gas)
- Native Go implementation of ERC-7562 tracer
- Minor relaxations to ERC-7562 validation rules
- Bug fixes from AA Bug Bounty Program
Description:
- EIP-7702 support integrates with the latest account abstraction proposal, allowing for more flexible and secure account management.
- ERC-712 support adopts standardized hashes and signatures for
UserOperations
, improving compatibility with external signers (like hardware wallets) and enhancing user experience. - The gas penalty adjustment eliminates penalties for unused gas below 40,000, making the system more user-friendly.
- A native Go implementation of the ERC-7562 tracer improves performance and integration for ERC-4337 bundlers.
- Relaxed ERC-7562 validation rules provide greater flexibility in processing UserOperations.
- Bug fixes from the AA Bug Bounty Program address vulnerabilities, boosting security and reliability.
MinimalDelegation Compatibility: Fully compatible with support for EIP-7702 authorizations.
Recommendation
Avoid using the
MinimalDelegation
with the v0.6.0 version of theEntryPoint
. The recommended version is the v0.8.0 as it includes support for EIP-7702 authorizations but v0.7.0 can also be used.Uniswap: Fixed in PR #149.
Cantina: Fix verified.
Use of unlicensed smart contracts
Description
All the smart contracts in the codebase are currently marked as unlicensed, as indicated by the SPDX license identifier at the top of the file:
// SPDX-License-Identifier: UNLICENSED
Using an unlicensed contract can lead to legal uncertainties and potential conflicts regarding the usage, modification and distribution rights of the code. This may deter other developers from using or contributing to the project and could lead to legal issues in the future.
Recommendation
It is recommended to choose and apply an appropriate open-source license to the smart contract. Some popular options for smart contract projects are:
- MIT License: A permissive license that allows for reuse with minimal restrictions.
- GNU General Public License (GPL): A copyleft license that ensures derivative works are also open-source.
- Apache License 2.0: A permissive license that provides an express grant of patent rights from contributors to users.
Uniswap: Fixed in PR #152.
Cantina: Fix verified.
When transferring 0 amount, ERC7914 is better to return true
Description
When transferring 0 amounts, ERC7914 returns false, which indicates that the transfer failed.
This makes ERC7914 behave like the ERC20 token that disallows 0 amount transfers, which has caused many integration issues.
When transferring 0 amount, in a sense it succeeds even if we do nothing, so returning true makes sense and can avoid integration issues.
Recommendation
function transferFromNative(address from, address recipient, uint256 amount) external returns (bool) {- if (amount == 0) return false;+ if (amount == 0) return true; _transferFrom(from, recipient, amount, false); emit TransferFromNative(address(this), recipient, amount); return true; } /// @inheritdoc IERC7914 function transferFromNativeTransient(address from, address recipient, uint256 amount) external returns (bool) {- if (amount == 0) return false;+ if (amount == 0) return true;
Uniswap: Fixed in PR #153.
Cantina: Fix verified.
_execute() may call handleAfterExecute() on stale hook
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
cccz
Description
In
_execute()
, it callshandleAfterExecute()
directly on hook that cached beforeto.call()
.IHook hook = settings.hook(); bytes memory beforeExecuteData; if (hook.hasPermission(HooksLib.BEFORE_EXECUTE_FLAG)) { beforeExecuteData = hook.handleBeforeExecute(keyHash, to, _call.value, _call.data); } (success, output) = to.call{value: _call.value}(_call.data); if (hook.hasPermission(HooksLib.AFTER_EXECUTE_FLAG)) hook.handleAfterExecute(keyHash, beforeExecuteData);
An edge case is if
to.call()
callsKeyManagement.update()
to update the setting, it may execute calls on stale hook.function update(bytes32 keyHash, Settings settings) external onlyThis { if (keyHash.isRootKey()) revert CannotUpdateRootKey(); if (!isRegistered(keyHash)) revert KeyDoesNotExist(); keySettings[keyHash] = settings; }
Recommendation
function _execute(Call memory _call, bytes32 keyHash) internal returns (bool success, bytes memory output) { // Per ERC7821, replace address(0) with address(this) address to = _call.to == address(0) ? address(this) : _call.to; Settings settings = getKeySettings(keyHash); if (!settings.isAdmin() && to == address(this)) revert IKeyManagement.OnlyAdminCanSelfCall(); IHook hook = settings.hook(); bytes memory beforeExecuteData; if (hook.hasPermission(HooksLib.BEFORE_EXECUTE_FLAG)) { beforeExecuteData = hook.handleBeforeExecute(keyHash, to, _call.value, _call.data); } (success, output) = to.call{value: _call.value}(_call.data);+ settings = getKeySettings(keyHash);+ hook = settings.hook(); if (hook.hasPermission(HooksLib.AFTER_EXECUTE_FLAG)) hook.handleAfterExecute(keyHash, beforeExecuteData); }
Uniswap: Acknowledged. We lean towards not fixing this because:
- A key would have to be an admin key to update its settings (and add a new hook)
- We enforce that functions related to key state are only accessible through execute, which applied execution hooks + checks key expiry
- Since the
afterExecute
hook function takes in the data returned from its correspondingbeforeExecute
hook call, we believe that there could be some edge cases where the hook could change after the low level call is made
validateUserOp() should not return early when the signature is invalid
Severity
- Severity: Informational
Submitted by
cccz
Description
According to the specs,
validateUserOp()
shouldn't return early even if the signature is invalid for gas estimation.- SHOULD not return early when returning SIG_VALIDATION_FAILED (1). Instead, it SHOULD complete the normal flow to enable performing a gas estimation for the validation function.
But in
MinimalDelegation.validateUserOp()
, the invalid signature causes early return and does not execute the following code, this makes it inconsistent with the specs.// If signature verification failed, return failure immediately WITHOUT expiry as it cannot be trusted if (!isValid) { return SIG_VALIDATION_FAILED; } Settings settings = getKeySettings(keyHash); _checkExpiry(settings); /// validationData is (uint256(validAfter) << (160 + 48)) | (uint256(validUntil) << 160) | (success ? 0 : 1) /// `validAfter` is always 0. validationData = uint256(settings.expiration()) << 160 | SIG_VALIDATION_SUCCEEDED; IHook hook = settings.hook(); if (hook.hasPermission(HooksLib.AFTER_VALIDATE_USER_OP_FLAG)) { // The hook can override the validation data validationData = hook.handleAfterValidateUserOp(keyHash, userOp, userOpHash, hookData); } }
And when the userOp is actually executed, the invalid signature will cause the transaction to revert in EntryPoint, so this will not cause the hook call to be actually executed.
function _validateAccountAndPaymasterValidationData( uint256 opIndex, uint256 validationData, uint256 paymasterValidationData, address expectedAggregator ) internal virtual view { (address aggregator, bool outOfTimeRange) = _getValidationData( validationData ); if (expectedAggregator != aggregator) { revert FailedOp(opIndex, "AA24 signature error"); }
Recommendation
- if (!isValid) {- return SIG_VALIDATION_FAILED;- } Settings settings = getKeySettings(keyHash); _checkExpiry(settings); /// validationData is (uint256(validAfter) << (160 + 48)) | (uint256(validUntil) << 160) | (success ? 0 : 1) /// `validAfter` is always 0. validationData = uint256(settings.expiration()) << 160 | SIG_VALIDATION_SUCCEEDED; IHook hook = settings.hook(); if (hook.hasPermission(HooksLib.AFTER_VALIDATE_USER_OP_FLAG)) { // The hook can override the validation data validationData = hook.handleAfterValidateUserOp(keyHash, userOp, userOpHash, hookData); }+ if (!isValid) {+ return SIG_VALIDATION_FAILED;+ }
Uniswap: Fixed in PR #154.
Cantina: Fix verified.