Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential withdrawal request griefing vector via permit front-running #803

Open
TheDZhon opened this issue Nov 10, 2023 · 3 comments
Open
Assignees
Labels
bug Something isn't working next-upgrade Things to pickup for the next protocol upgrade

Comments

@TheDZhon
Copy link
Contributor

TheDZhon commented Nov 10, 2023

Summary

Potential griefing vector of withdrawal requests having a pre-signed approval ('permit') signature attached
(e.g., no profit motive for an attacker but spoils UX for users of the protocol) by front-running the 'token.permit' calls with intercepted from mempool signatures.

Affected methods

The WithdrawalQueue contract is affected with the following withdrawal request methods:

These functions are designed to streamline transactions by combining token approval and withdrawal request operations into a single call.

Expected behavior

The requestWithdrawalsWstETHWithPermit and requestWithdrawalsWithPermit functions utilize the permit function so that approve and pull operations can happen in a single transaction instead of two consecutive transactions.

Griefing exploitation

ERC20Permit (stETH and wstETH inherit from this) uses the nonces mapping for replay protection. Once a signature is verified and approved, the nonce increases, invalidating the same signature being replayed.
The requestWithdrawalsWstETHWithPermit and requestWithdrawalsWithPermit functions expect the user to sign their tokens with the WithdrawlQueue contract address as spender and submit the transaction to the chain with the signature uint8 v, bytes32 r, bytes32 s as part of the arguments.
When the user transaction is in the mempool, an attacker can take this signature, call the token.permit function on the token themselves with the following arguments

address owner = victim address
address spender = WithdrawlQueue contract address

uint256 value = `_permit.value` copied from the still pending transaction
uint256 deadline = `_permit.deadline` copied from the still pending transaction
uint8 v = `_permit.v` copied from the still pending transaction
bytes32 r = `_permit.r` copied from the still pending transaction
bytes32 s = `_permit.s` copied from the still pending transaction

Since this is a valid signature, the token accepts it and increases the nonce.
This makes the user's transaction fail whenever it gets mined.

Potential impact

It's possible to induce requestWithdrawalsWstETHWithPermit and requestWithdrawalsWithPermit functions reverts via concurrent 3rd-party actions worsening UX.

Short-term mitigation

  1. The Lido staking widget mitigates the impact by refreshing allowances and allowing immediate retry of the failed tx with the requestWithdrawals method relying on the already provided allowance seamlessly.
  2. Guidance updates in the Lido tokens integration guide will recommend additional checks on the caller's code side to prevent exploitation.

Proposed long-term solution

Change the WithdrawalQueue implementation to be aware of the existing allowance at execution.

if (STETH.allowance(msg.sender, address(this)) < _permit.value) {
        STETH.permit(msg.sender, address(this), _permit.value, _permit.deadline, _permit.v, _permit.r, _permit.s);
}

This solution will be evaluated for integration in the next protocol update, targeted for Q2/Q3 2024 (tentatively).


This issue was reported responsibly through Immunefi and has been assigned a 'MEDIUM' severity rating under the stipulations of the Lido Bug Bounty Program. The individual initially identified the issue has received a reward concurrent with this publication.

@TheDZhon TheDZhon added bug Something isn't working next-upgrade Things to pickup for the next protocol upgrade labels Nov 10, 2023
@TheDZhon TheDZhon self-assigned this Nov 10, 2023
@grGred
Copy link

grGred commented Nov 10, 2023

Is it allowed to publicly speak about this issue? I would like to take a post about this with a deep dive, which might help other lsd protocols and auditors.

@TheDZhon
Copy link
Contributor Author

Is it allowed to publicly speak about this issue? I would like to take a post about this with a deep dive, which might help other lsd protocols and auditors.

Hello, @grGred

Since this is a public repo, please feel free to make any further references.

@TheDZhon
Copy link
Contributor Author

TheDZhon commented Jul 8, 2024

The article suggest another approach for mitigating the attack: https://www.trust-security.xyz/post/permission-denied
It might be more coherent to use as a long-term solution.

https://github.com/trust1995/trustlessPermit/tree/main

    function trustlessPermit(
        address token,
        address owner,
        address spender,
        uint256 value,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) internal {
        // Try permit() before allowance check to advance nonce if possible
        try IERC20Permit(token).permit(owner, spender, value, deadline, v, r, s) {
            return;
        } catch {
            // Permit potentially got frontran. Continue anyways if allowance is sufficient.
            if (IERC20(token).allowance(owner, spender) >= value) {
                return;
            }
        }
        revert("Permit failure");
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working next-upgrade Things to pickup for the next protocol upgrade
Projects
None yet
Development

No branches or pull requests

2 participants