`TwabERC20.permit()` can be easily griefed by anyone

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/TwabERC20.sol#L5 https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/TwabERC20.sol#L19 https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/TwabERC20.sol#L46 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/bd325d56b4c62c9c5c1aff048c37c6bb18ac0290/contracts/token/ERC20/extensions/ERC20Permit.sol#L49-L68

Vulnerability details

Impact

The TwabERC20.sol contract implements ERC20Permit, this library provides permit functionality for ERC20s but is left behind by the new reliable permit() standard that many major protocols have migrated to. The issue is that the (v,r,s) permit() signature exists in the mempool and can be easily executed directly on the token contract while frontrunning the permit() transaction.

Although I am aware that this cannot happen when using permit in the vault contract (with depositWithPermit), I think that TwabERC20.sol is an independent contract and therefore permit() in TwabERC20.sol should be overridden with the recommended.

Proof of Concept

The inherited ERC20Permit's permit function:

    function permit(
        address owner,
        address spender,
        uint256 value,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public virtual override {
        require(block.timestamp <= deadline, "ERC20Permit: expired deadline");

        bytes32 structHash = keccak256(abi.encode(_PERMIT_TYPEHASH, owner, spender, value, _useNonce(owner), deadline));

        bytes32 hash = _hashTypedDataV4(structHash);

        address signer = ECDSA.recover(hash, v, r, s);
        require(signer == owner, "ERC20Permit: invalid signature");

        _approve(owner, spender, value);
    }

For more information refer to the blog post on Trust Security's website.

Tools Used

Manual Review

Recommended Mitigation Steps

Use the trustlessPermit() variant here.

Assessed type

ERC20

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #13

[c4-judge]

hansfriese marked the issue as unsatisfactory: Out of scope