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.

[novaknole]

@pcaversaccio Hi Pascal,

I don't see what you mean by increaseAllowance/decreaseAllowance front-run. With approve way, if Bob currently has 100 tokens approved and Alice tries to approve(update it to) 50, Bob ends up with 100+50 = 150. So front-run is easy to understand here.

Now, let me explain my point of view about increaseAllowance. If Bob currently has 100 tokens approved and Alice calls increaseAllowance(Bob, 50), even if increaseAllowance is front-run, Bob can end up with max 150 tokens and tbh, even with front-run, nothing bad happened. Since Alice's reasoning was that she was increasing allowance by 50, she already knew that Bob would have 150 tokens possibility to gain.

The same goes with decreaseAllowance. If bob has 100 tokens approved and Alice calls decreaseAllowance(Bob, 30), then:

• if Bob front-runs this with transferFrom(Alice, Bob, 100), then allowance is updated to 0. Now decreaseAllowance(Bob, 30) is executed which will actually fail here
• if Bob front-runs this with transferFrom(Alice, Bob, 70), then allowance is updated to 30. Now decreaseAllowance(Bob, 30) is executed which sets allowance to 0.

In either way, Bob couldn't get his hands on more than 100 tokens.

Would appreciate the explanation how this increaseAllowance/decreaseAllowance doesn't solve the problem of "approve". Maybe I am not seeing something.

[pcaversaccio]

@novaknole I stated in the original issue description in brackets: "and decreaseAllowance can be frontrunned also". To be clear, I don't refer to increaseAllowance in any way in this statement. So what I mean by frontrunning in this context is the following:

1. Set allowance (via approve or increaseAllowance, it doesn't matter for this illustration) from A to B: 50.
2. A broadcasts a new Transaction to decrease the allowance for B by 10 using decreaseAllowance.
3. B observes the mempool and sees the action of A and frontruns it by spending 45. After this transaction, the remaining allowance from A to B is 5.
4. Since now we have currentAllowance < requestedDecrease, the transaction from A fails.

The reason why I mentioned it is that many people state that increaseAllowance / decreaseAllowance are frontrunning-safe which is simply not true for decreaseAllowance. Anyway, the entire frontrunning concern in this context is pointless. If you have given someone an ERC-20 token allowance, they can steal your money right away - there is no need for a race condition or anything similar. Thus, increaseAllowance and decreaseAllowance don't solve any real-world problems or security concerns.

[novaknole]

@pcaversaccio

There's one interesting thing. In your 4th bullet point, you correctly say that transaction from A fails. This is good(I know that A has to pay fees for gas, but that's just nothing compared to the problem in the approve way). In the approve way, if B had 50 allowance and A wanted to set it to 40(this is the same as decreaseAllowance(10)), then front-running here would result in B having 90 in the end, while with increaseAllowance/decreaseAllowance, B can't end up with more than 50 tokens.

Question 1: Don't you agree with what I just said ?

Question 2: Well, in my opinion, front-running with increaseAllowance/decreaseAllowance doesn't result in a tragedy, while in approve way, it does. Thoughts ?

[pcaversaccio]

Question 1: Don't you agree with what I just said ?

Question 2: Well, in my opinion, front-running with increaseAllowance/decreaseAllowance doesn't result in a tragedy, while in approve way, it does. Thoughts ?

Regarding approve I will put it straightforward here: ERC-20's approve function is a bug and not a feature. Besides horribly breaking the UX, it poses a serious security risk since you essentially delegate the proper risk management to a spender account. approve does break trustlessness in bad ways.

increaseAllowance and decreaseAllowance are completely unneeded, but opens up further attack vectors for scammers. I'm co-leading the SEAL 911 initiative, and I deal daily with scammed people due to increaseAllowance or permit. The true real security problem here is that unnecessary functions make it easier for people to fall victim to phishing. The amount of time and energy we as an industry spend on engineering complex applications and even cheer for it is one of the major bottlenecks for mainstream adoption. Complexity in engineering isn't a badge of honor, it's often a burden! So let's start cheering for simplicity!

[novaknole]

@pcaversaccio I completely agree with that. I just wanted to learn more and that's why I keep asking you the same question, but let me ask again.

assume that I am experienced. even if I am experienced, if I use approve method way, I can be hacked, but if I use increaseAllowance/decreaseAllowance, I won't be hacked, because it's clear that with those methods, the same hack doesn't happen because of the reasons stated in #4583 (comment). What is it that you don't agree here ?

Can you give an example of an attack of increaseAllowance ?

[pcaversaccio]

assume that I am experienced. even if I am experienced, if I use approve method way, I can be hacked, but if I use increaseAllowance/decreaseAllowance, I won't be hacked, because it's clear that with those methods, the same hack doesn't happen because of the reasons stated in #4583 (comment). What is it that you don't agree here ?

This attack is completely hypothetical, it never happens in practice. We even offered a bounty here #4583 (comment) for people showing the frontrun attack in the wild. Thus, this guardrail prevents nothing and just adds technical cruft.

Example attack via a phishing site:

• increaseAllowance called: https://pacific-explorer.manta.network/tx/0xc92dd0b055b3777fe91412acb646e030a876f5ab2508d1e43888653d290b5f01
• 10 seconds later the malicious token transfers happen (you have to look into the transaction trace to understand when the spender from the first step calls the token transfers): https://pacific-explorer.manta.network/tx/0x255e48b9be7af5d48278642286edd04741a027da9c9be68b1de8b7cebdeb411d

Many people are aware that they should be careful with approve but now we have an additional method increaseAllowance that people don't understand (permit has the same issue to be clear), and so many fail for this since they don't know how to verify the transaction.

[novaknole]

I think I understand your point. What I was saying was that increaseAllowance/decreaseAllowance solve the problem of double spending, while approve way doesn't even solve that. I think you agree with me till now, but then you say that because of these increaseAllowance/decreaseAllowance, people get scammed into signing the messages and that's why you don't like it, right ? My whole opinion was that I wanted you to agree with me about the fact that increaseAllowance/decreaseAllowance truly solve the double spending problem. Are we now on the same page ?

[pcaversaccio]

My whole opinion was that I wanted you to agree with me about the fact that increaseAllowance/decreaseAllowance truly solve the double spending problem. Are we now on the same page ?

Let me specify: I agree that these functions solve the hypothetical frontrunning issue. Apart from the arguments mentioned above, I also don't like these custom functions since they are not part of the EIP-20 specs or any other EIP. So they should not be part of any ERC-20 core library contract.

[novaknole]

Thanks for the help so much. One last question I have remaining is about phishing attack you mentioned.

so, malicious actor creates a dapp and users go there and dapp asks them to "sign" a increaseAllowance, but there's not a function like permit for increaseAllowance. So what message does a hacker ask a user to sign and what function does hacker run after this ?

[pcaversaccio]

Thanks for the help so much. One last question I have remaining is about phishing attack you mentioned.

so, malicious actor creates a dapp and users go there and dapp asks them to "sign" a increaseAllowance, but there's not a function like permit for increaseAllowance. So what message does a hacker ask a user to sign and what function does hacker run after this ?

they just ask for a signature for increaseAllowance, where the spender is usually an unverified contract. In the next block, they batch execute all possible transferFrom transfers that are possible due to the previous blocks approvals. This is usually done using a contract that batches the transactions together using a multicall approach. An example:

[novaknole]

I don't get how they ask a signature for increaseAllowance. Are they using eth_sign but that's deprecated by metamask.

Is this attack described somewhere ? I couldn't find anything on the internet.

[pcaversaccio]

I don't get how they ask a signature for increaseAllowance. Are they using eth_sign but that's deprecated by metamask.

Is this attack described somewhere ? I couldn't find anything on the internet.

The user makes an on-chain transaction with increaseAllowance. It's a normal contract interaction.

[novaknole]

haha. I thought attack was more complicated than that and I was going furious.

How is this attack any different than if hacker's dapp asks a user to send a transaction that transfers 1 eth from him to hacker's address ? I guess, a user can spot that he is losing 1 eth and will easily reject it while increaseAllowance is just a function that he has no idea of what it does. correct right ? I guess, I am looking at the situation from my point of view since I am a developer and have a hard time imagining myself instead of un-experienced user.

[novaknole]

@pcaversaccio what would be the best practice then for ERC20 contract to be integrated with dapp ? should we use approve(0), approve(amount) practice(that also doesn't solve the problem fully) or just integrate it normally as we did in the pass ? just simply call approve(amount) from dapp and leave it to the chance of what will happen ?