Open alex-ppg opened 1 year ago
i consider this to be pretty low severity as its entirely PEBCAK, likely a wontfix from me but will leave open in case others have a strong opinion
While the victimization is a PEBCAK issue (many are surprised it even works including me), block explorers as well as personal wallets being polluted with transactions that overpopulate transaction history is a nuisance. Additionally, the EIP-20 standard itself contains a SHOULD
clause which is not being enforced and logically should be. Just some additional points to consider.
Based on on-chain code analysis of existing DeFi projects, this change should never be applied as it can break existing patterns. For more information, consult: https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3931#issuecomment-1374793188
This is a duplicate of an issue I opened in the OpenZeppelin repository with regard to the address poisoning attack we have seen being repetitively exploited in the wild. As they are going to introduce a change for this particular issue, we believe it should be replicated in the
solmate
dependency as well given its popularity.📝 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 executingtransferFrom
instructions from arbitrary addresses with anamount
of0
.The problem arises from how
transferFrom
evaluates the approval between the caller and the sender of the funds. In the EIP-20 standard, the following statement is present: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
if
clause withintransferFrom
is refactored to enforce a non-zero approval between the sender of funds and themsg.sender
via arequire
check orif-revert
pattern, either of which is acceptable.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:
Invoking the
poison
function with arbitraryfrom_
andto_
arguments will successfully perform atransferFrom
invocation even when the approval betweenfrom_
(the sender of funds) andaddress(this)
(the caller of thetransferFrom
function) has been set to0
.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.