Improper use of the approve function can lead to front running attacks.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MToken.sol#L159-L164 https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/Well.sol#L89-L101

Vulnerability details

Impact

The current implementation of the approve() function within MToken.sol and Well.sol can potentially be front-runned under a certain set of circumstances. This is a well known bug in ERC20 contracts, and will be explained in detail below. The impact of this scenario is lost user funds. In particular, more mTokens and well tokens can be transferred out of user’s wallets than they intended.

Proof of Concept

The approve() function is called when a user permits the spender to transfer funds (via the transferFrom() function) on their behalf. The vulnerability presents itself when the initial user calls approve() a second time to re-adjust the allowance permitted to be spent. If the spender sees this re-adjustment transaction in the mempool before it gets mined, then the spender will have a brief opportunity to call transferFrom to transfer the initial balance (they can pay high gas fees to make sure their transfer gets mined first). The spender can then receive the original balance that they were permitted to spend, while still getting authorization to transfer the second re-adjusted balance. This would result in lost funds for the initial user.

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MToken.sol#L159-L164

function approve(address spender, uint256 amount) override external returns (bool) {
    address src = msg.sender;
    transferAllowances[src][spender] = amount;
    emit Approval(src, spender, amount);
    return true;
}

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/Well.sol#L89-L101

function approve(address spender, uint rawAmount) external returns (bool) {
    uint96 amount;
    if (rawAmount == type(uint).max) {
        amount = type(uint96).max;
    } else {
        amount = safe96(rawAmount, "Well::approve: amount exceeds 96 bits");
    }

    allowances[msg.sender][spender] = amount;

    emit Approval(msg.sender, spender, amount);
    return true;
}

Vulnerability in Steps:

1.  Alice calls approve(“0xBob”, 20) allowing Bob to transfer 20 tokens on her behalf.
2.  Alice decides to re-adjust the allowance so that Bob can only transfer 10 tokens.  She does this 
by calling approve(“0xBob”, 10).
3.  Bob takes note of this new transaction before it gets mined, and swiftly calls 
transferFrom(“0xAlice”, “0xBob”, 20).  He pays a high gas fee so that his transaction gets mined 
before Alice's transaction.  This allows him to receive the 20 tokens that he was originally 
permitted to transfer.
4.  Once Alice’s new transaction (which allows Bob to transfer 10 tokens) gets mined, Bob once again 
calls transferFrom(“0xAlice”, “0xBob”, 10), allowing him to receive the 10 tokens.
5.  All-in-all, Bob receives 30 of Alice’s tokens, even though Alice never intended for him to 
transfer that amount.

Recommended Mitigation Steps

The solution for both mToken.sol and Well.sol are virtually the same. Both contracts should implement the increaseAllowance and decreaseAllowance functions below.

The two functions for mToken.sol:

function increaseAllowance(address spender, uint256 addedValue) public virtual returns (bool) {
    address src = _msgSender();
    originalAllowance = _transferAllowances[src][spender];
    _transferAllowances[src][spender] = originalAllowance + addedValue;
    return true;
}

function decreaseAllowance(address spender, uint256 requestedDecrease) public virtual returns (bool) 
{
    address src = _msgSender();
    originalAllowance = _transferAllowances[src][spender];
    require(originalAllowance >= requestedDecrease, "requested decrease exceeds current allowance");
    unchecked {
        _transferAllowances[src][spender] = originalAllowance - requestedDecrease;
    }
    return true;
}

The two functions for Well.sol:

function increaseAllowance(address spender, uint256 addedValue) public virtual returns (bool) {
    originalAllowance = allowances[msg.sender][spender];
    require(originalAllowance + addedValue >= type(uint96).max, "approve amount exceeds 96 bits");
    allowances[msg.sender][spender] = originalAllowance + addedValue;
    return true;
}

function decreaseAllowance(address spender, uint256 requestedDecrease) public virtual returns (bool) 
{
    originalAllowance = allowances[msg.sender][spender];
    require(originalAllowance >= requestedDecrease, "requested decrease exceeds current allowance");
    unchecked {
        allowances[msg.sender][spender] = originalAllowance - requestedDecrease;
    }
    return true;
}

Furthermore, the line of code below should also be added as a check inside of the approve() function so that users can only call approve() if they are adding a transfer allowance for the first time. If they need to change this allowance, then users will need to do this via the increaseAllowance() or decreaseAllowance() function. Under this scenario, front running attacks will result in the initial transferAllowance being transferred before the allowance can be changed, but it won’t result in additional mTokens or well tokens being transferred beyond what users initially approved.

require(_transferAllowances[src][spender] == 0);

Assessed type

ERC20

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[ElliotFriedman]

non issue, this happens in all ERC20 tokens

[c4-sponsor]

ElliotFriedman marked the issue as sponsor disputed

[alcueca]

If anyone asserts that the approve front-run vulnerability is valid, I will charge them with contempt of court.

[c4-judge]

alcueca marked the issue as unsatisfactory: Insufficient proof