transferFrom should validate approval before transfer

[cleanunicorn]

Description

Should validate approval before sending tokens, especially since there is the possibility to implement a hook.

Recommendation

Prefer to update (and check) approval

https://github.com/akiratechhq/review-halo-dao-lending-market-2021-10/blob/fbfed0a187e9d8df75172a17a83d6cafbb5cbc8a/code/contracts/incentives/lib/ERC20.sol#L119-L123

Before the actual transfer of tokens

https://github.com/akiratechhq/review-halo-dao-lending-market-2021-10/blob/fbfed0a187e9d8df75172a17a83d6cafbb5cbc8a/code/contracts/incentives/lib/ERC20.sol#L118

[andreiashu]

I couldn't find any specific rationale for changing the order of the operations in the OpenZeppelin's code. The change was made several years ago and there doesn't seem to have had a clear reason for it:

https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1609/files#diff-fa792f7d08644eebc519dac2c29b00a54afc4c6a76b9ef3bba56c8401fe674f6R85-R89

Original PR discussion: OpenZeppelin/openzeppelin-contracts#1604

[andreiashu]

One thing that does change is the order of events emitted. If this change is implemented then the Approve event will be emitted before the Transfer one.

[andreiashu]

closing for now. It's a strange choice to do the transfer accounting before the approval one but it has been like this since more than 2 years ago according to Open Zeppelin's github history.
We've asked through some of our channels (Twitter) and waiting for more information.