0xSwahili - The exaTokens ERC20 token-contract approve function is prone to a front-run attack

[sherlock-admin2]

0xSwahili

high

The exaTokens ERC20 token-contract approve function is prone to a front-run attack

Summary

exaTokens token owners are prone to a double allowance spend due to an approval-race condition on the token contract

Vulnerability Detail

Users are allowed to withdraw assets from an account owner when the owner calls approve function on the Market contract and grants them an allowance:

function approve(address spender, uint256 amount) public virtual returns (bool) {
        allowance[msg.sender][spender] = amount;

        emit Approval(msg.sender, spender, amount);

        return true;
    }

This allowance can be used by the beneficiary to withdraw owners assets, eg by calling withdrawAtMaturity:

  function withdrawAtMaturity(
    uint256 maturity,
    uint256 positionAssets,
    uint256 minAssetsRequired,
    address receiver,
    address owner
  ) external whenNotPaused returns (uint256 assetsDiscounted) {

However, the approve method is subject to an approval-race attack that can allow malicious beneficiaries to "double spend" owners tokens.

Impact

exaToken owners are likely to assign "double allowances" to beneficiaries against general expectation.

Code Snippet

Consider this POC:

Alice deposits some assets onto a Market and is minted some exaTokens. Alice wishes to appoint Bob to operate on her assets and decides to grant Bob some allowance by calling approve(Bob,100);. After some time, Alice decides to reduce Bobs allowance to 50. Bob, who is actively monitoring the mempool, spots Alice's transaction. He launches a sandwich attack on Alice transaction via MEV that will:

1. Withdraw Alice tokens using up the initial allowance of 100 by calling withdrawAtMaturity.
2. Allow Alice approve transaction to be minted, in effect "adding" more allowance to 50.
3. Withdrawing Alice tokens using the new allowance of 50 by calling withdrawAtMaturity

The end result is that Bob spends 150 instead of 50. This is not what Alice wished.

Tool used

Manual Review

Recommendation

To fix this vulnerability, I recommend that the exaTokens approval function be modified such that any Non-Zero token approvals calls must first be preceded by a Zero amount approval call, similar to the USDT token contract found here:

https://etherscan.io/token/0xdac17f958d2ee523a2206206994597c13d831ec7#code#L199

/**
    * @dev Approve the passed address to spend the specified amount of tokens on behalf of msg.sender.
    * @param _spender The address which will spend the funds.
    * @param _value The amount of tokens to be spent.
    */
    function approve(address _spender, uint _value) public onlyPayloadSize(2 * 32) {

        // To change the approve amount you first have to reduce the addresses`
        //  allowance to zero by calling `approve(_spender, 0)` if it is not
        //  already 0 to mitigate the race condition described here:
        //  https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
        require(!((_value != 0) && (allowed[msg.sender][_spender] != 0)));

        allowed[msg.sender][_spender] = _value;
        Approval(msg.sender, _spender, _value);
    }

[santipu03]

The approve race condition is no longer considered an issue due to the lack of exploitability in the wild. Moreover, OZ deprecated the use of increaseAllowance and decreaseAllowance in the newer 5.0 version of their contracts because they didn't consider that frontrunning approvals were an issue anymore.

Check this for reference:

1. Discussion to remove increaseAllowance and decreaseAllowance from ERC20 OpenZeppelin/openzeppelin-contracts#4583
2. Remove non-standard increaseAllowance and decreaseAllowance from ERC20 OpenZeppelin/openzeppelin-contracts#4585