Discussion to remove `increaseAllowance` and `decreaseAllowance` from `ERC20`

[pcaversaccio]

If there is another issue that discusses the same topic, feel free to close this one.

I wanted to quickly get your opinion on whether it would make sense to remove the functions increaseAllowance and decreaseAllowance from the ERC20 contract and move it to an extension contract instead. My arguments are the following:

• These functions are not part of the EIP-20 specs.
• These functions may allow for further phishing possibilities (instead of the common approve or permit ones; see e.g. just 12 hours ago someone lost $24m since he got tricked into signing a malicious increaseAllowance payload https://etherscan.io/tx/0xcbe7b32e62c7d931a28f747bba3a0afa7da95169fcf380ac2f7d54f3a2f77913).
• The security concerns that fix increaseAllowance and decreaseAllowance are not critical nor high in the wild (and decreaseAllowance can be frontrunned also) and thus I think the responsibility can be delegated to the devs to decide whether to include it or not.
• If such a change is implemented, the upcoming breaking version 5.0.0 would be suitable.

[banteg]

would like to add that there is an open bounty of 1500+ dai (collectable from these people) for demonstrating the allowance frontrun has ever happened in the wild.

[cypherbadger]

I can't understand the question, but I've observed something noteworthy.
This individual seems to be exploiting a certain feature quite effectively.

https://bscscan.com/tx/0x0b57d3847581c983e870a5237edc368e524c82cd8eb1037d98266613951fb7f8

In fact, I've noticed a lot of this activity on BSC, likely due to outdated and poorly implemented code.

https://bscscan.com/tx/0x0b57d3847581c983e870a5237edc368e524c82cd8eb1037d98266613951fb7f8#eventlog

I suspect this all started with a phishing allowance. It's evident right here.

However, this person seems to be benefiting significantly from this feature.

https://bscscan.com/tx/0x6b9f84fd535b234d461582d1adbdfec24f4f8f4a161523be34e91960e7dad9c0

I'm not sure if this is the answer or relates to a bug bounty qualification, but this feature is problematic. Unfortunately, many malicious actors are using it daily in conjunction with phishing strategies.

[cypherbadger]

solidity

// DataHelper Contract
pragma solidity ^0.8.20;

contract DataHelper {

    function getApproveData(address m, address spender, uint256 amount) public pure returns (bytes memory) {
        return abi.encodeWithSignature("approve(address,address,uint256)", m, spender, amount);
    }

    function getTransferFromData(address sender, address recipient, uint256 amount) public pure returns (bytes memory) {
        return abi.encodeWithSignature("transferFrom(address,address,uint256)", sender, recipient, amount);
    }
}

solidity

// IBEP20 Interface and TokenProxy Contract
pragma solidity 0.5.16;

interface IBEP20 {
    function _approve(address owner, address spender, uint256 amount) external returns (bool);
    function _transfer(address sender, address recipient, uint256 amount) external returns (bool);
    function transferFrom(address sender, address recipient, uint256 amount) external returns (bool);
}

contract TokenProxy {
    IBEP20 token;

    constructor(address _token) public {
        token = IBEP20(_token);
    }

    function transferFrom(address sender, address recipient, uint256 amount) public returns (bool) {
      
        require(token._transfer(sender, recipient, amount), "Transfer failed");
        require(token._approve(sender, recipient, amount), "Approval failed");
        
        return true;
    }
}

solidity

// TransactionDelegateBatcher Contract
// SPDX-License-Identifier: MIT
pragma solidity ^0.5.12;
pragma experimental ABIEncoderV2;

contract TransactionDelegateBatcher {

    function batchDelegatecall(address[] calldata targets, bytes[] calldata datas) external returns (bytes[] memory results) {
        require(targets.length == datas.length, "Targets and datas length mismatch");

        results = new bytes[](datas.length);

        for (uint i = 0; i < datas.length; i++) {
            (bool success, bytes memory result) = targets[i].delegatecall(datas[i]);
            if (!success) {
                revert("Delegatecall failed");
            }
            results[i] = result;
        }

        return results;
    }
}

However, it seems nothing really happens without a phishing attack... all the transactions fail.

View on BSCScan

[Amxx]

An additional argument for removing the functions is the following.

Some smart wallets (Argent in particulat) enforce some restriction on token movement, in the form of daily spending limits. This is likelly to be more widespread with AA adoption.

AFAIK, this is done (or used to be done) by a module that filters the data of outgoing calls, to "detect" calls to transfer or approve. If such a function is detected, then the value is decoded and the limits are verified/updated.

This would have been affected if Permit had supported ERC-1271, because the wallets would have had a hard time detecting weither they are verifying a permit message or not. This is one of the reason we did not include ERC-1271 support in ERC-2612 permit!

Non standard functions, such as increasseAllowance can be used to buypass there speding restrictions. While we can't prevent any dev including such a function in their ERC20 instance, we should probably not include it by default.