Signatures can be held indefinitely, as they cannot expire

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-02-uniswap-foundation/blob/main/src/UniStaker.sol#L512-L532

Vulnerability details

Impact

Any function that uses a signature to act on behalf of the owner can be postponed indefinitely, as signatures do not have any deadline.

This can lead to issues, as the action can be executed at any time in the future (possibly at an unfavorable moment for the signer), and the signer has no means to invalidate a signature.

Proof of Concept

This issue applies to any function that uses _revertIfSignatureIsNotValidNow. This includes:

• stakeOnBehalf
• stakeMoreOnBehalf
• alterDelegateeOnBehalf
• alterBeneficiaryOnBehalf
• withdrawOnBehalf
• claimRewardOnBehalf

Let's take for example withdrawOnBehalf:

	function withdrawOnBehalf(
	    DepositIdentifier _depositId,
	    uint256 _amount,
	    address _depositor,
	    bytes memory _signature,
	  ) external {
	    Deposit storage deposit = deposits[_depositId];
	    _revertIfNotDepositOwner(deposit, _depositor);

	    _revertIfSignatureIsNotValidNow(
	      _depositor,
	      _hashTypedDataV4(
	        keccak256(
@>	          abi.encode(WITHDRAW_TYPEHASH, _depositId, _amount, _depositor, _useNonce(_depositor))
	        )
	      ),
	      _signature
	    );

	    _withdraw(deposit, _depositId, _amount);
	  }

There are no checks in place to ensure that a withdrawal will be executed within a reasonable timeframe. Someone having this signature can essentially wait indefinitely, and proceed with the withdrawal at an unfavorable moment without any means for the owner to revoke or invalidate the signature.

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding a deadline parameter inside the hash. This is an example for withdrawals, but it should be applied to any function that uses _revertIfSignatureIsNotValidNow:

	function withdrawOnBehalf(
	    DepositIdentifier _depositId,
	    uint256 _amount,
	    address _depositor,
	    bytes memory _signature,
+	    uint32 deadline	    
	  ) external {
+	    require(block.timestamp <= deadline, "expired signature");

	    Deposit storage deposit = deposits[_depositId];
	    _revertIfNotDepositOwner(deposit, _depositor);

	    _revertIfSignatureIsNotValidNow(
	      _depositor,
	      _hashTypedDataV4(
	        keccak256(
-	      abi.encode(WITHDRAW_TYPEHASH, _depositId, _amount, _depositor, _useNonce(_depositor))
+	      abi.encode(WITHDRAW_TYPEHASH, _depositId, _amount, _depositor, _useNonce(_depositor), deadline)
	        )
	      ),
	      _signature
	    );

	    _withdraw(deposit, _depositId, _amount);
	  }

Assessed type

Timing

[c4-judge]

MarioPoneder marked the issue as primary issue

[c4-sponsor]

apbendi (sponsor) confirmed

[c4-sponsor]

apbendi marked the issue as disagree with severity

[apbendi]

Adding a deadline is a good idea, and we will likely implement it. That said, the degree to which this could be actively used in an attack is limited, it can't allow the holder of the signature to steal funds or take any action the user didn't express consent for, but only submit it at "an inopportune time". Additionally, the user would always be able to mitigate even that risk by simply moving assets out of the wallet in question, or taking another action via signature which would consume the nonce. There is no opportunity for griefing, as the nonce would be consumed and the holder of the signed message couldn't replay it, but only execute it once.

[MarioPoneder]

Deadline checks as well as slippage checks can be considered state-of-the art and a protocol bears a responsibility to introduce such measures in order to protect its users.
However, as the sponsor already pointed out, the potential impact is Low as the signature can only be used at an unfavorable time only if the creator of the signature hasn't invalidated it already. This is always possible by creating another signature and using it oneself, which would consume the nonce.

Nevertheless, this is a valid & valuable QA suggestion.

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by MarioPoneder

[c4-judge]

MarioPoneder marked the issue as satisfactory

[c4-judge]

MarioPoneder marked the issue as selected for report

[apbendi]

We'd like to reiterate that, while we find this report helpful, we don't believe it rises to the level of Medium severity, as explained above. We'd also like to expand on the mitigation avenue available in the contracts as written:

Several the of the onBehalf methods can be called with values that make them effectively a no-op. For example, the alterBeneficiaryOnBehalf method can be called with the existing beneficiary. Or the stakeMore method can be called with 0. This means the user has an effective way to invalidate the nonce used in the signature that is being withheld in a potentially malicious manner. Combined with the fact that the nature of the UniStaker contracts means delayed execution cannot do any real "damage" to the user beyond minor inconvenience, we see this as an Information or at worst low severity issue.

[wildmolasses]

here are a few contests where the "missing deadline checks" were reported as medium severity:

• https://code4rena.com/reports/2022-12-backed#m-01-missing-deadline-checks-allow-pending-transactions-to-be-maliciously-executed
• https://code4rena.com/reports/2022-12-caviar#m-01-missing-deadline-checks-allow-pending-transactions-to-be-maliciously-executed
• function _compoundFees does not allow user to supply deadline 2023-05-maia-findings#171

these are cases where the contract seems to forward a swap to some uni pool. i think our case is much different though. in our case, the time sensitivity is not nearly as material. like, is the worst case here that you changed your mind about staking X amount of UNI but then an operator went ahead and did it for you a day later anyway? ok, where's the leaked value? are assets at risk? i think the answer is no and no, distinguishing this case from the others.

[DadeKuma]

but then an operator went ahead and did it for you a day later anyway? ok, where's the leaked value?

Hello, and thank you for your comments! Given your questions, even if the contest has ended, I think I can provide some additional info to further validate the severity of this issue.

Consider the following cases as a leak of value:

1. The user is forced to make a new transaction to increase the nonce and cancel his signature; otherwise, the signature cannot be invalidated, it will linger forever. Given that this contract will be deployed on Mainnet, this is a leak of value, as gas fees are expensive.

2. Remember that the UNI token has monetary value and can be traded. A user might need to withdraw his stake within a reasonable amount of time to take advantage of price volatility; otherwise, they would prefer to keep it staked to earn staking rewards. If the withdrawal is executed after the volatility period has ended, the user has to make a new transaction to stake it again. Plus, they will miss out on rewards during this period between these two transactions, as they can't control the maximum time when it will be executed.

3. The UNI token can indeed be volatile, as we can see on Coinmarketcap. If we look at the 1-year view, we can see a recent example: between 02/23/2024 and 02/24/2024, the price jumped from $7.34 to $11.36. So yes, even a single day can make a difference.

[wildmolasses]

hi @DadeKuma

1. The user is forced to make a new transaction to increase the nonce and cancel his signature; otherwise, the signature cannot be invalidated, it will linger forever. Given that this contract will be deployed on Mainnet, this is a leak of value, as gas fees are expensive.

The signature will be invalidated when the nonce is used. This comment assumes a malicious relayer will grief you by withholding your onBehalf payloads. You can instead find a different relay operator.

2. Remember that the UNI token has monetary value and can be traded. A user might need to withdraw his stake within a reasonable amount of time to take advantage of price volatility; otherwise, they would prefer to keep it staked to earn staking rewards. If the withdrawal is executed after the volatility period has ended, the user has to make a new transaction to stake it again. Plus, they will miss out on rewards during this period between these two transactions, as they can't control the maximum time when it will be executed.

i don't think this is relevant to the issue at hand; including a deadline in an onBehalf method does not imply that some relayer will send your transaction. if a user needs to withdraw her stake in some specific time period, she should send the transaction herself, or use some trusted relayer to submit the tx on her behalf. enforcing a deadline on the payload does not protect from malicious relayers withholding txs.

3. The UNI token can indeed be volatile, as we can see on Coinmarketcap. If we look at the 1-year view, we can see a recent example: between 02/23/2024 and 02/24/2024, the price jumped from $7.34 to $11.36. So yes, even a single day can make a difference.

Again, in the case where a malicious relayer withholds your withdrawal tx, the user gets no protection from an enforced deadline in the payload. The user should protect herself here by calling withdraw herself, or finding a better relay operator.

[dmvt]

Summary of the issue

Functions that use signatures to act on behalf of the owner could be used against the owner's will, as signatures do not have any deadline.

Picodes view

The current main argument for Medium severity is: Deadline checks (similar to slippage checks) are state-of-the-art and the protocol bears a responsibility to provide such protection measures for users.

I am not convinced by this argument. To me, it's true only when the signature is used to swap on a pool or to do an inherently time-sensitive action. Otherwise, it's a valid design to view signatures as transactions and to expect users to have an understanding of how nonces work if they want to invalidate them. Or if we push the reasoning, we could argue that the fact that transactions have no deadline is a Medium severity issue at the chain level?

So to me, this should be QA or even Invalid.

gzeon’s view

Agree with Picodes here, deadline only protect against pending tx where the outcome of the transaction would vary by time (e.g. price movement). All the listed functions there have deterministic outcomes and should not require a deadline. If a signature needs to be revoked, the user can burn a nonce then invalidate it.

QA / Invalid (feature request) imo

LSDan’s view

I view this as outright invalid. The core premise that the signature doesn't expire is misleading and bordering on outright false. The nonce causes signatures to expire, making a longstanding signature the result of a careless user no longer wanting something they allowed for to happen but taking no action to prevent it.
ref: https://docs.code4rena.com/awarding/judging-criteria/supreme-court-decisions-fall-2023#verdict-severity-standardization-conditional-on-user-mistake

Result

Issue #205 will be invalidated.

[MarioPoneder]

Agree. See also: #417 (comment)