Race condition on ERC20 approval

[code423n4]

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/YUSDToken.sol#L253-L259

function _approve(address owner, address spender, uint256 amount) internal {
    assert(owner != address(0));
    assert(spender != address(0));

    _allowances[owner][spender] = amount;
    emit Approval(owner, spender, amount);
}

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/YETI/YETIToken.sol#L206-L209

function _approve(address owner, address spender, uint256 amount) internal {
    _allowances[owner][spender] = amount;
    emit Approval(owner, spender, amount);
}

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/YETI/sYETIToken.sol#L133-L137

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

Using approve() to manage allowances opens yourself and users of the token up to frontrunning. Best practice, but doesn't usually matter.

Explanation of this possible attack vector

See also: 0xProject/0x-monorepo#850

A potential fix includes preventing a call to approve if all the previous tokens are not spent through adding a check that the allowed balance is 0:

require(allowed[msg.sender][_spender] == 0).

[kingyetifinance]

Duplicate #19

[alcueca]

Taking as main

[alcueca]

While this attack theoretically can lead to loss of funds, it has never been observed in the wild despite innumerable opportunities for it to happen. Downgrading it to non-critical, and might start marking it as invalid in future contests.