[M-04] LiquidityPool.requestDepositWithPermit(): Request deposits with permit can be broken for asset tokens that do not follow ERC2612 standard permit function

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L220-L226
https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/ERC20.sol#L216-L237

Vulnerability details

Impact

In the LiquidityPool.requestDepositWithPermit(), the implementation invokes the underlying token's permit() function and proceeds with the assumption that the operation was successful, without checking the outcome.

LiquidityPool.sol#L220-L226

function requestDepositWithPermit(uint256 assets, address owner, uint256 deadline, uint8 v, bytes32 r, bytes32 s)
    public
{
    ERC20PermitLike(asset).permit(owner, address(investmentManager), assets, deadline, v, r, s);
    investmentManager.requestDeposit(assets, owner);
    emit DepositRequested(owner, assets);
}

The ERC20.permit() implemented is almost identical to the OZ version of the permit() function here.

ERC20.sol#L216-L237

function permit(address owner, address spender, uint256 value, uint256 deadline, bytes memory signature) public {
    require(block.timestamp <= deadline, "ERC20/permit-expired");
    require(owner != address(0), "ERC20/invalid-owner");

    uint256 nonce;
    unchecked {
        nonce = nonces[owner]++;
    }

    bytes32 digest = keccak256(
        abi.encodePacked(
            "\x19\x01",
            block.chainid == deploymentChainId ? _DOMAIN_SEPARATOR : _calculateDomainSeparator(block.chainid),
            keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, value, nonce, deadline))
        )
    );

    require(_isValidSignature(owner, digest, signature), "ERC20/invalid-permit");

    allowance[owner][spender] = value;
    emit Approval(owner, spender, value);
}

However, it is known that certain tokens such as DAI utilizes a permit() function here that deviates from the reference implementation in ERC20.sol. The differences are outlined here. The lack of verification could possibly lead to unexpected behavior when interacting with non-conforming tokens.

DAI Token

function permit(address holder, address spender, uint256 nonce, uint256 expiry,
                bool allowed, uint8 v, bytes32 r, bytes32 s) external
{
    bytes32 digest =
        keccak256(abi.encodePacked(
            "\x19\x01",
            DOMAIN_SEPARATOR,
            keccak256(abi.encode(PERMIT_TYPEHASH,
                                    holder,
                                    spender,
                                    nonce,
                                    expiry,
                                    allowed))
    ));

    require(holder != address(0), "Dai/invalid-address-0");
    require(holder == ecrecover(digest, v, r, s), "Dai/invalid-permit");
    require(expiry == 0 || now <= expiry, "Dai/permit-expired");
    require(nonce == nonces[holder]++, "Dai/invalid-nonce");
    uint wad = allowed ? uint(-1) : 0;
    allowance[holder][spender] = wad;
    emit Approval(holder, spender, wad);
}

In addition, tokens containing phantom permits (e.g. WETH) are known to exist, which would not revert on failure. Hence, setting a precedent of explicitly reverting when calling unsupported permit functions can help ensure requestDepositWithPermit() do not suffer from calling phantom permits on the underlying currency assets.

Tools Used

Manual Analysis

Recommendaiton

Add proper verification to the permit() function call. This could be done by using a safePermit() function within ERC20.sol similar to the one OZ implemented here instead of a permit() function. (For more details you can check this discussion)

Assessed type

Context

[c4-pre-sort]

raymondfam marked the issue as low quality report

[raymondfam]

N-40 from the bot.

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Out of scope

[nevillehuang]

My mistakes on repeating the phantom permit functions part. However, the automated bot report does not mention integration of DAI as possible currency asset for liquidity pools, so that part of the submission should still be valid. The only other report mentioning this is #462. #509 is actually invalid since it is indeed mentioning the phantom permits similar to the finding in N-40.

There is a similar finding here. It is explicitly mentioned in README that stable coins are accepted. Also, here is a example RWA centrifuge pool that utilizes DAI as the underlying currency.

While I understand that governance decides on the stablecoin/tokens to add, unless Centrifuge is planning to not support currencies with non-compliant permit function like DAI (which is not made known in the contest details), the first part of the finding should still be valid.

[c4-judge]

gzeon-c4 removed the grade

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)

[gzeon-c4]

Valid but QA, it is impossible to support every token if they are non-compliant. @hieronx