fix(ERC6909): fail when from is msg.sender with no approval on transferFrom

[egozoq]

Description

Currently, when you call transferFrom where from is msg.sender where msg.sender is neither operator nor has any allowance, it succeeds. This should not be the case.

Here's a quote below taken directly from the EIP (https://eips.ethereum.org/EIPS/eip-6909):

MUST revert when the caller is not an operator for the sender and the caller’s allowance for the token id for the sender is insufficient.

MUST revert when the sender’s balance for the token id is insufficient.

As we can see from above, the standard does not state that a transferFrom should be approved when from = msg.sender. You strictly need an operator or approval for the function to go through, according to the EIP.

We also take note that for the ERC20.sol file's transferFrom call, the call will fail if there are no approvals, even if from is msg.sender (so same call fails for ERC20, but not for ERC6909 - wording is the same for both EIPs, afaik). So I would argue that need an approve for ERC6909 regardless (not for consistency per se, but because of how the EIP was written/worded).

The fuzzing error that caught this:

[FAIL. Reason: assertion failed; counterexample: calldata=0xc5b21e8f0000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e14960000000000000000000000003e6e9d3f7520f7b7fc0cc9511a573a9b2b64f2d80000000000000000000000000000000000000000000003bfb05b8917d487df6c000000000000000000000000000000000000000000000000000000000000023a args=[0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496, 0x3e6E9d3f7520f7b7FC0Cc9511a573a9b2B64f2d8, 17703135468196458913644 [1.77e22], 570]] testFailTransferFromNotAuthorized(address,address,uint256,uint256) (runs: 5313, μ: 38677, ~: 38677)

---

There's another fuzzing error that surfaces from testTransferFromWithApproval, that stems from the same issue. The problem with this error is that we don't decrease the allowance, even if the allowance value is not type(uint256).max and the caller is not an operator. The one-liner also amends this problem.

To quote that this is indeed an error, w.r.t. the EIP's wording:

MUST decrease the caller’s allowance by the same amount of the sender’s balance decrease if the caller is not an operator for the sender and the caller’s allowance is not infinite.

SHOULD NOT decrease the caller’s allowance for the token id for the sender if the allowance is infinite.

SHOULD NOT decrease the caller’s allowance for the token id for the sender if the caller is an operator.

We read that the caller's allowance must decrease (we only do not increase in case of infinite allowance or the caller is an operator). But, it does not, due to the msg.sender != sender condition inside transferFrom.

The fuzzing error:

Failing tests:
Encountered 1 failing test in src/test/ERC6909.t.sol:ERC6909Test
[FAIL. Reason: assertion failed; counterexample: calldata=0xaad06d3d0000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e1496000000000000000000000000fbacd3b7998d569c966fcb2631f26f2d0252104600000000000000000000000000000000000000000000000000000000000001a65221d29b24d0b2a8f0e347ad3137d1473c4b279db8419ac2137268ade13050fb0000000000000000000000000000000000000000000000000000000000000a13 args=[0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496, 0xfBACd3b7998D569C966fcb2631f26f2d02521046, 422, 37149413086367185788713642480572377364906692461899534453171704069506267631867 [3.714e76], 2579]] testTransferFromWithApproval(address,address,uint256,uint256,uint256) (runs: 288, μ: 89289, ~: 94719)

Checklist

Ensure you completed all of the steps below before submitting your pull request:

• Ran forge snapshot?
• Ran npm run lint?
• Ran forge test?

Pull requests with an incomplete checklist will be thrown out.

[egozoq]

Not totally related, but it seems that solady follows the standard correctly, and does not allow transferFrom when from is msg.sender, I believe.

https://github.com/Vectorized/solady/blob/c6738e40225288842ce890cd265a305684e52c3d/src/tokens/ERC6909.sol#L232