Disallow Address Poisoning in ERC20

[alex-ppg]

💻 Environment

Environment is not applicable to this bug report as it pertains to the smart contracts themselves and how they behave on-chain.

📝 Details

The current token contract implementation of the EIP-20 standard does not satisfy all SHOULD clauses of the standard definition and as such permits the commonly-known address poisoning attack by executing transferFrom instructions from arbitrary addresses with an amount of 0.

The problem arises from how _spendAllowance evaluates the approval between the caller and the sender of the funds. In the EIP-20 standard, the following statement is present:

The function SHOULD throw unless the _from account has deliberately authorized the sender of the message via some mechanism

This condition is not validated for zero-value transfers as no "deliberate" approval is evaluated. To ensure we remain compliant with the EIP-20 standard in full, we cannot disallow zero-value transfers altogether.

As a workaround, we propose that the require check within _spendAllowance is refactored to evaluate a non-zero approval between the sender of funds and the caller.

This change will ensure maximal compatibility with existing contract systems, conform to the EIP-20 standard to a greater degree, and address the address poisoning attack we have seen being extensively exploited in recent times.

🔢 Code to reproduce bug

A simple Solidity smart contract to illustrate the bug in action:

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract Poisoning {
    ERC20 public immutable token;

    constructor() {
        token = new ERC20("Test", "TST");
    }

    function poison(address from_, address to_) external {
        require(token.allowance(from_, address(this)) == 0);
        token.transferFrom(from_, to_, 0);
    }
}

Invoking the poison function with arbitrary from_ and to_ arguments will successfully perform a transferFrom invocation even when the approval between from_ (the sender of funds) and address(this) (the caller of the transferFrom function) has been set to 0.

This behaviour permits polluting the transfer history of an account on blockchain explorers which in turn can cause users to be misled and copy incorrect addresses when performing their own valid transfers.

[frangio]

Thanks @alex-ppg. This was on our radar as well.

I don't see this as a compliance issue because the EIP-20 spec is not at all clear due to the contradicting statements about transfers of 0 value and deliberate authorization, and also because "SHOULD" doesn't mean "MUST".

That said, I do agree that we should prevent address poisoning in our implementation.

As a workaround, we propose that the require check within _spendAllowance is refactored to evaluate a non-zero approval between the sender of funds and the msg.sender in case the amount is being specified as 0.

Not sure I understand the suggestion here.

What we are considering is essentially adding require(currentAllowance != 0) in spendAllowance. Is this what you mean?

[alex-ppg]

Thanks for the swift response @frangio, evaluating just the currentAllowance as non-zero is an adequate remediation to the issue. My proposal was to do it conditionally only when amount_ was 0, but evaluating it as non-zero outright is identical and would actually incur less of a gas overhead. Looking forward to seeing this implemented sooner than later as it should be a small change.

[alex-ppg]

Updated original issue to illustrate only the need for require(currentAllowance != 0), thanks for the feedback @frangio

[Amxx]

The simplest way to implement this would probably be to update the _spendAllowance function like this:

function _spendAllowance(address owner, address spender, uint256 amount) internal virtual {
    uint256 currentAllowance = allowance(owner, spender);
    if (currentAllowance != type(uint256).max) {
-       require(currentAllowance >= amount, "ERC20: insufficient allowance");
+       require(currentAllowance >= Math.max(amount, 1), "ERC20: insufficient allowance");
        unchecked {
            _approve(owner, spender, currentAllowance - amount);
        }
    }
}

Still, we are worried that this breaking change could cause real composability issues. DeFi is so big, its difficult for us to understand which legitimate workflows, if any, this might break. We don't want the fix to cause more issues than the original problem.

We should keep in mind that this poisoning attack is not an onchain vulnerability. Assets are safe. Its an issue that affect offchain interpretation of the logs. If we are to change onchain code to fix offchain missunderstandings, we better make sure that we are not breaking onchain workflows in the process.

[alex-ppg]

An alternative to this would be re-evaluating the rationale behind EIP-20's MUST keyword for firing an event in case of a zero-value transfer.

Honestly, the use-case of such zero-value events does not appear to provide any real value off-chain. The EIP-20's edit history indicates that the language was using should before it was changed to MUST, however, I was not able to identify any additional background about why this change was made.

Not emitting the Transfer event for zero-value transferFrom invocations seems like an adequate solution to solve this issue without affecting on-chain code whatsoever.

[Amxx]

An alternative to this would be re-evaluating the rationale behind EIP-20's MUST keyword for firing an event in case of a zero-value transfer.

Strongly disagree.

EIP-20 is final. It has been for a long time, and there is no mechanism to update it. Even if we could update it, you would not be able to change the behavior of the many non-upgradeable contracts already onchain.

Keep in mind that triggering the event is not the issue here. The issue is in the way offchain tools show these events, and in the way users trust the data they see on these offchain tools.

If you want to change something that has been here for years, I would argue we should change etherescan (to not show these 0 value transfers by default, unless a toggle is activated) rather than changing EIP-20.

[pcaversaccio]

I would like to quickly highlight that OZ has always been focused on getting the on-chain behavior right in first place. That's why for example my PR was not accepted https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3381. To be consistent with the past, I would strongly advocate talking to off-chain tools like Etherscan & co to fix that issue... (for instance, there is an advanced view for internal tx about zero value transactions - the same approach could be taken here for token transfers).

[alex-ppg]

Strongly disagree.

EIP-20 is final. It has been for a long time, and there is no mechanism to update it. Even if we could update it, you would not be able to change the behavior of the many non-upgradeable contracts already onchain.

Understood, however, the same is true if we update the ERC20 dependency of OpenZeppelin to satisfy the SHOULD clause of EIP-20. My advice was not to change an optional SHOULD clause to MUST, it would have been to change a MUST clause (emission of events) to a SHOULD clause which would have retained all presently deployed contracts compliant.

Additionally, the problem is not the zero-value transfers themselves. The problem is that a transfer can be shown as originating from a user who never authorized anyone to perform it. This exact case is covered in the existing EIP-20 albeit via a SHOULD clause.

Arguably, block explorers must remain impartial in the data they showcase to users (as they are a data tool). As such, transfers of zero are in-fact acceptable behaviour under EIP-20 that should be shown.

When I saw zero-value transfers originating from my account, my first reaction was that my keys were compromised and that the software I was seeing glitched in showing zero as the value. Transfers originating from an account that never approved the caller is something illogical and must be prohibited (IMO of course).

The core problem lies in the SHOULD clause which specifies that:

The function SHOULD throw unless the _from account has deliberately authorized the sender of the message via some mechanism

This clause was never taken into account for zero-value transfers when building the OpenZeppelin ERC-20 contract and as such is not satisfied.

[Amxx]

My advice was not to change an optional SHOULD clause to MUST, it would have been to change a MUST clause (emission of events) to a SHOULD clause which would have retained all presently deployed contracts compliant.

That is exactly what I believe we should not do

• To interpret/change a SHOULD as a MUST, and do in the code what the EIP leaves up to the implementer is ok
• To interpret/change a MUST as a SHOULD, and take the liberty to not do what the EIP mandates is not ok

[Amxx]

The function SHOULD throw unless the _from account has deliberately authorized the sender of the message via some mechanism

This was considered, but clearly not as strictly as one could expect.

It would be wrong to say we don't do it. It would also be wrong to say we cannot be more strict and interpret the EIP in a more restrictive way ... ... and I would do it if I was convinced this more restrictive interpretation would not break anything.

My concern is that being more strict in our interpretation might be both

• actually not mandated by the EIP
• breaking some DeFi composability

[alex-ppg]

With regards to DeFi composability, this is not a retroactive change and as such would solely affect the compatibility of OZ ERC-20 tokens with existing protocols.

The most serious impact this change can have is on protocols that perform a transferFrom invocation with from_ being a smart contract that is incapable of increasing the approval of the caller smart contract.

While protocols performing transferFrom even when the amount is 0 are not seldom (i.e. best-effort reward distribution mechanisms), the 0 amount is never deliberate and the code has been prepared to mostly handle non-zero values. As such, a way to increase approval again is expected to be built in regardless of whether from_ is an EOA or a smart contract.

Given that performing a transferFrom with a value of 0 (or even a transfer) is a waste of gas for most smart contract systems, new projects developed with OZ ERC-20 tests will be mindful of this and, in reality, benefit from more optimal code.

Additionally, systems are already mindful of a few caveats with ERC-20 tokens (a non-zero approval over existing approval, the non-mandatory requirement to yield a bool etc.) and this would become another addition to the list albeit one that would not need to be circumvented most of the time as deliberate zero-value transfers are mostly undesirable.

I believe this change would collectively lead to:

• a reduction of on-chain data bloat as transactions would not be able to be spammed from unauthorized addresses
• a better-defined transferFrom behaviour that would cause developers to write more optimal code

[Amxx]

The most serious impact this change can have is on protocols that perform a transferFrom invocation with from_ being a smart contract that is incapable of increasing the approval of the caller smart contract.

Yes, My concern is that there could be pair of contracts (router + liquidity pool ?) where one contract does the approve and the other does transferFrom. They would work in tandem, and use the same amount. If that amount is non-zero, it will continue to work properly, but if the amount is 0, then it would suddenly fail.

The amount could be 0 for many reason. Imagine a balancer-like pool that is supposed to have 3 tokens, but only actually contains 2. You try to withdraw. This includes non-zero amount for 2 of the 3 tokens and zero for the third one.

Its really an edge case, but that might happen somewhere in the big world of DeFi

[Amxx]

• a reduction of on-chain data bloat as transactions would not be able to be spammed from unauthorized addresses

This is really opinionated. The whole concept of "spam by unauthorized addresses" is strange. The addresses are paying gas to use the block space. You main not like what they do, but the whole point of a decentralized censorship-resistant blockchain is that they are free to pay gas fee to do whatever they want, including sending (many) empty tx to themselves or to anyone.

[Amxx]

• a better-defined transferFrom behaviour that would cause developers to write more optimal code

My concern is not with the more optimal code that developers will write in the future, its with the (less optimal?) code that was written in the past, and that was written in a way that is expected to work with all upcoming EIP-20 contracts.

We really don't want to be the ones that create a split between the EIP-20 tokens that work with X and the EIP-20 tokens that don't work with X.

[alex-ppg]

This is really opinionated. The whole concept of "spam by unauthorized addresses" is strange. The addresses are paying gas to use the block space. You main not like what they do, but the whole point of a decentralized censorship-resistant blockchain is that they are free to pay gas fee to do whatever they want, including sending (many) empty tx to themselves or to anyone.

Just a quick note, the problem is not "sending (many) empty tx to themselves or to anyone". Even if this issue's proposed change is accepted, users will be able to spam zero-value transfers to themselves / any other account.

The problem lies in that they are able to impersonate a transaction as if it was coming from your (or anyone's) personal account. In a decentralized censorship-resistant blockchain, identities need to be secure and impossible to impersonate.

If this was not true, I would be able to send a zero-value transfer originating from your account to someone else's. This is impossible unless I have access to your private key. This issue is about applying the same principle to ERC-20 tokens (being deliberately approved to perform a transfer before doing so).

[Amxx]

That is fair.

I personally don't see an EIP-20 transfers as "transactions by the from", but I see how less technical users could see it that way.

[DanielVF]

At first I was excited about blocking zero approval transferFroms, but after more thinking, I think it's a bad idea. As @Amxx said, the core issue is exact amount approve and transferFrom pairs of code. These uses shouldn't require adding extra code when to avoid borking up a protocol.

[frangio]

the core issue is exact amount approve and transferFrom pairs of code

I share the abstract concern but it would also be great if someone could point out a specific piece of code that would break in some cases.

[DanielVF]

I just looked on GitHub, and here's a random contract using the pattern of approve amount then call other contract.

        TransferHelper.safeApprove(DAI, address(swapRouter), amountIn);

        ISwapRouter.ExactInputSingleParams memory params = ISwapRouter
            .ExactInputSingleParams({
                ...
                amountIn: amountIn,
                ...
            });

https://github.com/bijaybogati/swap-app-demo/blob/68494ecd8ab9179fe62468be4ff3c2ef350a240f/swap-app/src/smart_contract/contracts/Swap.sol#L51-L65

This is a fairly common pattern. As simple swap of zero funds reverting may not be problem when standing alone, but when this pattern is built into other contracts, it can be. For example a DeFi system that harvests and swaps each coin in a list could revert and block all the others if one happened to be zero.

[alex-ppg]

Hey @DanielVF, thanks for having a look. The pattern you shared is incorrect, it would continue functioning as expected after the change.

The snippet showcases a pattern whereby a swap is performed. Performing a swap of zero is not an expected scenario as all exchanges apply fees and would be unable to do so.

This is further showcased by the Uniswap V3 system that the code integrates with as it already fails on zero swaps.

I understand people are hesitant to make the change, however, based on the logical analysis a few responses above I fail to see a valid scenario in which a transferFrom with zero is expected to succeed when no approval has been previously made.

To clarify, this issue will permit zero-value transfers to occur as per the EIP-20 standard. It will solely affect transferFrom operations of a value of zero that are performed by an address that never had any allowance to begin with.

[pcaversaccio]

FWIW, Curve's founder @michwill mentioned to me that some old Curve contracts might break. Tagging him here since it's important enough.

[alex-ppg]

I am performing an analysis of the top projects in the space to identify what this change would mean to them. After validating that Uniswap implementations are unaffected, I have identified that Compound would be affected. We can see what this change would cause:

• Breaking Code: The CDaiDelegate implementation performs a doTransferIn call with an argument of 0. This solely affects the DAI implementation (as only CDaiDelegate performs this) and would manifest as an issue if the DAI implementation was upgraded to use the new OpenZeppelin version. In the current implementation, the doTransferIn function right now performs a transfer of zero to self, wasting gas on each invocation when it should have conditionally performed the transfer (in which case; more optimal code + code would not break).

• Affected Code: The doTransferIn function is the sole piece of code that executes a transferFrom instruction. This function is invoked in several segments of the codebase, namely: mintFresh, repayBorrowFresh, _addReservesFresh. -- mintFresh: This function is invoked by the top-level mint function and would fail if the supplied amount is zero. In the current implementation, it results in a no-op. -- repayBorrowFresh: This is in turn invoked by liquidateBorrowFresh, repayBorrowInternal, and repayBorrowBehalfInternal. The first instance already does not allow an amount of 0 whilst the other two instances would fail if the supplied amount is zero. In the current implementation, they result in a no-op. -- _addReservesFresh: This function is invoked by the top-level _addReserves function and would fail if the supplied amount is zero.

I believe the above is sufficient evidence to not proceed with the change. All instances that would break by this issue would actually be resolved by proper programming practices (i.e. minimizing gas by performing transfers only when needed).

However, given that many projects do not follow them including some of the most renowned ones like Compound, the OpenZeppelin library should not change. The only change that can be performed without altering on-chain code is affecting off-chain code and namely the Transfer event emission.

I understand that EIPs are final (even more so an EIP like 20 which has been utilized for multiple years and is the "prime example" of a smart contract EIP), however, it was finalized before the Ethereum community was as large as it is today. As the event emission would only affect off-chain code, I believe that not emitting it only in unauthorized transferFrom invocations is not a ground-breaking change and would resolve the issue at hand.

Feel free to close this issue if you do not intend to make such a change.

[pcaversaccio]

Another contract (Curve.fi: Compound Swap) that would be affected is this one here. See function remove_liquidity_imbalance.

[alex-ppg]

@pcaversaccio That contract would not be affected. Again, this solely affects transferFrom invocations and not all that are performed with a value of 0. In the code you reference, a transfer is performed which will succeed with a value of 0 as we are not concerned about those.

I believe add_liquidity would fail, however, as it performs a transferFrom for all of the tokens when a user can supply liquidity using only one token AFAIK. This means that the Curve model performs two redundant transfer operations wasting gas on each liquidity supply if it is performed only in one token...

[pcaversaccio]

@pcaversaccio That contract would not be affected. Again, this solely affects transferFrom invocations and not all that are performed with a value of 0. In the code you reference, a transfer is performed which will succeed with a value of 0 as we are not concerned about those.

You're actually right - got confused while switching between transfer and transferFrom searches on Etherscan.

I believe that not emitting it only in unauthorized transferFrom invocations is not a ground-breaking change

I honestly don't know how many people built monitoring systems that depend on tracking such transfer events (maybe to get insights about other stuff). So in that case, I would advocate more for: leaving it as it is since it works so far (apart from the poisoning issue ofc).

[alex-ppg]

After giving it a lot more thought, I think that this issue should be closed. If we remain as is, we force off-chain explorers to adjust their off-chain code to not showcase zero-value transfers.

If we perform the event emission change, we will reduce the gas cost of zero-value transactions but would in turn break the EIP-20 standard as well as potentially force the other spectrum of off-chain code to change that relies on zero-value transfers.

IMO either of those two is sub-optimal and the best would have been to force deliberate authorization but we are too late for that. I leave it up to the OZ maintainers to choose whether they will perform a change (for events) or close the issue as is.

[frangio]

If we remain as is, we force off-chain explorers to adjust their off-chain code to not showcase zero-value transfers.

The thing is they should do this anyway for the thousands of tokens that already have this behavior.

Thanks everyone for contributing with concrete examples. We haven't come to a conclusion yet.

[frangio]

We've decided against making any changes, due to the risk that they would cause issues across the ERC20 ecosystem as discussed earlier in this issue.

We are open to reconsidering this if this is seen as an important measure with low risk. Please feel free to continue the discussion here if you disgree with the risk assessment so far.