Add a safePermit function for ensuring reverting calls to ERC20.permit()

[hjorthjort]

🧐 Motivation
The Multichain/AnySwap exploits were caused by smart contracts incorrectly assuming that a failed permit call would always revert. This is similar to how a smart contract might assume that calls to transfer or approve will revert if they fail. For this, OpenZeppelin's SafeERC20 library has the safeTransfer, safeTransferFrom, safeApprove, etc.

📝 Details
ERC2612 permit functions do not return any value to indicate success, they only revert. However, a "phantom function" call behaves exactly like a successful call, unless state is inspected.

ERC2612 tokens maintain and expose a mapping of nonces, and a successful call to permit MUST increment the nonce of the owner address. This state update can be observed to ensure that a permit function has successfully been called.

The suggested file for this would be contracts/token/ERC20/utils/SafeERC20Permit.sol.

import "../extensions/draft-IERC20Permit.sol";

library SafeERC20Permit {

    function safePermit(
        IERC20Permit token,
        address owner,
        address spender,
        uint256 value,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) internal {
        uint256 nonce = token.nonces(owner);
        token.permit(owner, spender, value, deadline, v, r, s);
        uint256 newNonce = token.nonces(owner);
        require (newNonce == nonce + 1, "SafeERC20Permit: permit did not succeed");
    }
}

[frangio]

Thanks for sharing the idea @hjorthjort. I think it makes sense to have a safePermit and your suggested mplementation looks good to me. I'd like to hear other opinions as well.

It's worth noting that if permit is a phantom function then token.nonces(owner) is likely going to revert anyway because the returndata will be empty and won't decode to a uint.

[duckki]

I think it's a good protective measure to call nonces (if permit() call returned no value). However, I don't think it's feasible to verify the correctness of external token (by checking nonce before and after). Just calling once is enough to avoid undefined permit issue.
Also, adding a bool return value to the permit specification would help optimize safePermit implementions like this:

    // Note: Pseudo-code
    function safePermit( ... ) {
        (success, data) = token.call(permit's hash, owner, spender, value, deadline, v, r, s);
        if (data.length == 0) {
            // compatibility mode
            // double check whether the token actually implements ERC-2612's `nonces` function.
            token.nonces(owner); // The return value is intentionally ignored.
        }
        else {
            // If "permit()" call returns a bool value, we can avoid making another external call.
            require( abi.decode(data, (bool)), ...); // check the return value.
        }
    }

[hjorthjort]

Thanks for the suggestion.

I think it's unlikely to expect the permit specification to be updated. But if we can, then great!

I'm not a big fan of the "if the call succeeds then the function is implemented" approach. It is generally not safe to assume a function will revert if it is not specifically implemented. Relying on contracts to not return any value for specific calls (instead of checking their semantic specification) feels like kicking the can down the road. But, I know this approach is used in safeTransfer for ERC20s, so maybe that makes sense as a consistency measure. But safeTransfer on ERC721s actually performs an external call (way more dangerous and costly).

In the case of EIP-2612 we have a simple measure in the fact that the semantics of nonces are extremely clear. In EIP-20 there is plenty of room for interpretation on how the contract should be updated. I think that since here we have a way to be more safe at a small gas cost, we should.

[Amxx]

I wonder why you would check the nonce (needs two read) rather than checking the allowance (only one read needed).

After all, I'm not sure we care about the nonce behavior (might work differently) as long as the approval is set correctly, right?

[frangio]

Checking allowance is not enough to verify that a permit was used successfully, because the allowance may just happen to be the same value contained in the permit.