code-423n4 / 2023-07-tapioca-findings

12 stars 9 forks source link

Cross-chain replay attacks are possible #1594

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/markets/MarketERC20.sol#L251-L284

Vulnerability details

Impact

In MarketERC20.sol we have _permit() function:

function _permit( /
        bool asset, // 1 = asset, 0 = collateral
        address owner,
        address spender,
        uint256 value,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) internal {
        require(block.timestamp <= deadline, "ERC20Permit: expired deadline");

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

        bytes32 hash = _hashTypedDataV4(structHash);

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

        if (asset) {
            _approve(owner, spender, value);
        } else {
            _approveBorrow(owner, spender, value);
        }
    }

In this function, the signed data doesn't include the chainId and because of this cross-chain replay attacks are possible.

Proof of Concept

According to EIP4337 standard to prevent replay attacks the signature should depend on chainId. But in these functions, chainId is not used. A valid signature that was used on one chain could be copied by an attacker and propagated onto another chain.

Tools Used

Manual review

Recommended Mitigation Steps

Add chainId _permit() function structHash.

bytes32 structHash = keccak256(
            abi.encode(
                asset ? _PERMIT_TYPEHASH : _PERMIT_TYPEHASH_BORROW,
                owner,
                spender,
                value,
                _useNonce(owner),
                deadline,
                block.chainId
            )
        );

Assessed type

Other

c4-pre-sort commented 1 year ago

minhquanym marked the issue as duplicate of #281

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Invalid