maxim371 - Potential for allowance front-running in transferFrom function

[sherlock-admin3]

maxim371

Medium

Potential for allowance front-running in transferFrom function

Summary

The transferFrom function is susceptible to the well-known ERC20 allowance front-running attack. If a user wants to change their allowance from a non-zero value to another non-zero value, an attacker can front-run the transaction and spend the old allowance + the new allowance

Root Cause

https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/lockstake/src/LockstakeMkr.sol#L79C5-L103C6

function transferFrom(address from, address to, uint256 value) external returns (bool) {
    require(to != address(0) && to != address(this), "LockstakeMkr/invalid-address");
    uint256 balance = balanceOf[from];
    require(balance >= value, "LockstakeMkr/insufficient-balance");

    if (from != msg.sender) {
        uint256 allowed = allowance[from][msg.sender];
        if (allowed != type(uint256).max) {
            require(allowed >= value, "LockstakeMkr/insufficient-allowance");

            unchecked {
                allowance[from][msg.sender] = allowed - value;
            }
        }
    }

    unchecked {
        balanceOf[from] = balance - value;
        balanceOf[to] += value; // note: we don't need an overflow check here b/c sum of all balances == totalSupply
    }

    emit Transfer(from, to, value);

    return true;
}

https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/lockstake/src/LockstakeMkr.sol#L79C5-L103C6

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

Initial state:

Alice (token holder) has 1000 tokens Alice has approved Bob (spender) to spend 100 tokens

Alice decides to change Bob's allowance to 50 tokens:

Alice submits a transaction to call approve(Bob, 50)

Bob (the attacker) sees this transaction in the mempool and quickly submits two transactions with a higher gas price:

Transaction 1: transferFrom(Alice, Bob, 100) (using the old allowance) Transaction 2: transferFrom(Alice, Bob, 50) (using the new allowance)

The transactions are processed in this order due to Bob's higher gas price:

Bob's Transaction 1 executes, transferring 100 tokens from Alice to Bob Alice's approve transaction executes, setting Bob's allowance to 50 Bob's Transaction 2 executes, transferring another 50 tokens from Alice to Bob

Result:

Bob has successfully transferred 150 tokens from Alice, even though Alice only intended to allow a maximum of 100 tokens (the original allowance) or 50 tokens (the new allowance)

Impact

Users will lose their money. An attacker can front-run the transaction and spend the old allowance + the new allowance

PoC

No response

Mitigation

To mitigate this issue, one common solution is to implement an increaseAllowance and decreaseAllowance function instead of approve. Another approach is to first set the allowance to 0 and then to the desired value in two separate transactions, but this is less convenient for users.

`function increaseAllowance(address spender, uint256 addedValue) public returns (bool) { allowance[msg.sender][spender] += addedValue; emit Approval(msg.sender, spender, allowance[msg.sender][spender]); return true; }

function decreaseAllowance(address spender, uint256 subtractedValue) public returns (bool) { uint256 currentAllowance = allowance[msg.sender][spender]; require(currentAllowance >= subtractedValue, "ERC20: decreased allowance below zero"); unchecked { allowance[msg.sender][spender] = currentAllowance - subtractedValue; } emit Approval(msg.sender, spender, allowance[msg.sender][spender]); return true; }`

[telome]

This is a known "issue" and these non-standard methods were purposefully not included in any of the new tokens. See for example the discussion in 3.1.1 in https://github.com/makerdao/nst/blob/dev/audit/20231124-cantina-report-review-makerdao-nst.pdf

Note in particular that LockstakeMkr is a token used internally by the LSE and is not transferrable directly by users, hence these non-standard methods would not even be callable, were they to be added.