Centrifuge - 0xpiken's results

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/ERC20.sol#L48

Vulnerability details

Impact

Due to wrong calculation of _DOMAIN_SEPARATOR, ERC20.permit() may not verify signature signed by signer. The design is supposed to support offline token approving but can not work as expected.

Proof of Concept

When calculating _DOMAIN_SEPARATOR during tranche token deployment, the field name is also included, but the value is not set in constructor(). It only has correct value when ERC20.file("name", name) is called.

From the user's perspective, they will create signature following EIP-2612, the name of the tranche token will be used for _DOMAIN_SEPARATOR calculation and the result will be different from the value stored in tranche token smart contract, ERC20.permit() will always be reverted because of this incorrect calculation.

Tools Used

Manual review

Recommended Mitigation Steps

Pass name in ERC20.constructor() other than assigning it's value through ERC20.file("name", name):

constructor(string memory _name, string memory _symbol, uint8 decimals_) {
    name = _name,
    symbol = _symbol,
    decimals = decimals_;
    wards[_msgSender()] = 1;
    emit Rely(_msgSender());

    deploymentChainId = block.chainid;
    _DOMAIN_SEPARATOR = _calculateDomainSeparator(block.chainid);
}

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/PoolManager.sol#L214-L225 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/ERC20.sol#L216-L237

Vulnerability details

Impact

ERC20.sol#permit() uses immutable _DOMAIN_SEPARATOR to verify signed message unless block.chainid is changed. If PoolManager#UpdateTrancheTokenMetadata() is called, ERC20.name will be modified. Any signed message thereafter will use new value of name, but the old value is still been used in ERC20.sol#permit() and cause it reverted.

Proof of Concept

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/ERC20.sol#L228 We can see that domain separator is recalculated only after block.chainid changed. The immutable _DOMAIN_SEPARATOR will be used no mater how name is modified. Any signed message calculating with new value of name will be identified as invalid message.

Tools Used

Manual review

Recommended Mitigation Steps

Recalculate domain separator when name changed:

contract ERC20 is Context {
    //@audit-info use to store initial hash of name
    bytes32 private immutable nameHash;
    ...
    constructor(uint8 decimals_) {
        ...
        _DOMAIN_SEPARATOR = _calculateDomainSeparator(block.chainid);
        //@audit-info calculate initial hash of name
        nameHash = keccak256(bytes(name));
    }
    function permit(address owner, address spender, uint256 value, uint256 deadline, bytes memory signature) public {
        ...
        //@audit-info recalculate domain separator when name changed
        bytes32 digest = keccak256(
            abi.encodePacked(
                "\x19\x01",
                (block.chainid == deploymentChainId && nameHash == keccak256(bytes(name))) ? _DOMAIN_SEPARATOR : _calculateDomainSeparator(block.chainid),
                keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, value, nonce, deadline))
            )
        );]
        ...
    }
}

Assessed type

Other

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/LiquidityPool.sol#L237-L243

Vulnerability details

Impact

Anyone can call ERC20PermitLike(token).permit() to verify signed message and update corresponding allowance mapping. The signed message can only be permitted one time, any attempt to permit it again will be reverted. Hence malicious user could frontrun ERC20PermitLike(token).permit() with signed message to block calling of LiquidityPool#requestDepositWithPermit() or LiquidityPool#requestRedeemWithPermit().

Proof of Concept

Add below codes before line 944 of LiquidityPool.t.sol and run test case, LiquidityPool#requestDepositWithPermit() will be reverted:

erc20.permit(investor, address(investmentManager), amount, block.timestamp, v, r, s);

Add below codes before line 991 of LiquidityPool.t.sol and run test case, LiquidityPool#requestRedeemWithPermit() will be reverted:

trancheToken.permit(investor, address(investmentManager), maxMint, block.timestamp, v, r, s);

Tools Used

Manual review

Recommended Mitigation Steps

Check allowance before calling permit() in LiquidityPool#requestDepositWithPermit() and LiquidityPool#requestRedeemWithPermit() as Uniswap did:

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

function requestRedeemWithPermit(uint256 shares, address owner, uint256 deadline, uint8 v, bytes32 r, bytes32 s)//@audit-ok
    public
{
    if (share.allowance(owner, address(investmentManager)) < shares) {
        share.permit(owner, address(investmentManager), shares, deadline, v, r, s);//@audit-info check if signature is signed by owner and combined by all parameters.
    }
    investmentManager.requestRedeem(shares, owner);//@audit-info requestRedeem if signature is valid
    emit RedeemRequested(owner, shares);
}

Assessed type

Other