Openzeppelin-contracts: Securence of ERC20 transferFrom

[andrewerf]

Please explain, what prevents such kind of attack ?

Imagine Alice calls approve(Contract.address, amount) to invoke Contract' method() which withdraws from her address.
Then imagine Bob has accidentally found it out, and he wants to prevent Alice invokes method().
So he waits until Alice' approve transaction is mined, then calls transferFrom(Alice.address, Contract.address, amount) and pushes it faster then Alice pushes method(). In the end, Contract receives money, but method cannot be invoked because allowance now is zero. And so money is lost for Alice.

[Skyge]

@andrewerf Hello! actually, when Alice calls

approve(Contract.address, amount)

it means only Contract can transfer token instead of Alice, so only Contract can call

transferFrom(Alice.address, Contract.address, amount)

if Bob calls this approach transferFrom(), you can find at here: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L99, so it will fail due to not satisfying the conditions.

[Skyge]

By the way, For support questions (rather than Feature Requests or Bug Reports),
as @abcoathup 's suggestion that you can ask in the : OpenZeppelin Community Forum that way the entire community can help answer your question.

[roleengineer]

@andrewerf Hello! actually, when Alice calls

approve(Contract.address, amount)

it means only Contract can transfer token instead of Alice, so only Contract can call

transferFrom(Alice.address, Contract.address, amount)

if Bob calls this approach transferFrom(), you can find at here: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L99, so it will fail due to not satisfying the conditions.

Hi, I'm not an expert, and maybe answer on my question lays somethere in the docs, but I can't understand why firstly executes internal function _transfer and secondly _approve here
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L99, if only _approve throws the error and reverts tx. Do you think there is no situation when the _transfer function will success before tx reverts?

[Skyge]

@roleengineer I have asked the same question yesterday, I think someone made a mistake in a pr, at least at the original version, it will check

require(_value <= allowed[_from][msg.sender]);

at first.

[nventuro]

Do you think there is no situation when the _transfer function will success before tx reverts?

No, the whole transaction must go through or be reverted. Additionally, there's no transfer of control during _transfer (as opposed to e.g. a regular Solidity transfer, which may execute a fallback function), so an attack has no control over execution flow.