lidofinance / core

Lido DAO smart contracts
https://lido.fi
GNU General Public License v3.0
382 stars 194 forks source link

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

Open TheDZhon opened 1 year ago

TheDZhon commented 1 year ago

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.

grGred commented 1 year ago

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 commented 1 year ago

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 commented 4 months ago

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");
    }