Replay Attack because EIP712 DOMAIN_SEPARATOR stored as immutable

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/ocean/ERC1155PermitSignatureExtension.sol#L17-L18 https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/ocean/ERC1155PermitSignatureExtension.sol#L40 https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/ocean/OceanERC1155.sol#L36-L39 https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/ocean/Ocean.sol#L79

Vulnerability details

Impact

Loss of fund due to replay attacks. Approvals made on one chain could be replayed when there is a fork without owner's consent.

Proof of Concept

The issue is in the ERC1155PermitSignatureExtension.sol which is inherited by the OceanERC1155.sol and subsequently inherited by the Ocean.sol contract which is in scope.

EIP712 spec incorporates the chainID into signed data to prevent replay attacks but the ERC1155PermitSignatureExtension.sol's implementation caches the DOMAIN_SEPARATOR variable as immutable variable. The cached DOMAIN_SEPARATOR immutable variable is used in the setPermitForAll to _setApprovalForAll of owner's ERC1155 without considering the actual chainId in case of a hard fork.

The immutable DOMAIN_SEPARATOR becomes hardcoded with the chainID in the constructor. And since there is no dynamic function to get the DOMAIN_SEPARATOR based on chainId as implemented by Openzeppelin's EIP712, the hardcoded chainID in the DOMAIN_SEPARATOR is always used for all chains in case of hard forks. Thus creating vulnerability to replay attacks.

This implementation of EIP712 is unsafe as all signatures to approve owner's ERC1155s will be valid in all forked networks causing owner's to lose all their ERC1155s in all forked networks since the signature is used to _setApprovalForAll owner's ERC1155s.

abstract contract ERC1155PermitSignatureExtension {
    /// @notice EIP-712 Ethereum typed structured data hashing and signing
    bytes32 public immutable DOMAIN_SEPARATOR;
    bytes32 public immutable SETPERMITFORALL_TYPEHASH;

    /// @notice Nonces used for EIP-2612 sytle permits
    mapping(address => uint256) public approvalNonces;

    constructor(bytes memory name, bytes memory version) {
        bytes memory EIP712Domain =
            bytes("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");
        DOMAIN_SEPARATOR = keccak256(
            abi.encode(keccak256(EIP712Domain), keccak256(name), keccak256(version), block.chainid, address(this))
        );
        SETPERMITFORALL_TYPEHASH =
            keccak256("SetPermitForAll(address owner,address operator,bool approved,uint256 nonce,uint256 deadline)");
    }

    function setPermitForAll(
        address owner,
        address operator,
        bool approved,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    )
        external
    {
        require(_signaturesEnabled(), "Permit Signature Disabled");
        require(deadline >= block.timestamp);
        bytes32 digest = keccak256(
            abi.encodePacked(
                "\x19\x01",
                DOMAIN_SEPARATOR,
                keccak256(
                    abi.encode(SETPERMITFORALL_TYPEHASH, owner, operator, approved, approvalNonces[owner]++, deadline)
                )
            )
        );
        address recoveredAddress = ecrecover(digest, v, r, s);
        require(recoveredAddress != address(0) && recoveredAddress == owner);
        _setApprovalForAll(owner, operator, approved);
    }
...
}

Tools Used

EIP712 specification and Openzepelin's implementation of EIP712

• https://eips.ethereum.org/EIPS/eip-712
• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/EIP712.sol

Recommended Mitigation Steps

In order to prevent replay attack and as best practice, adhere to the EIP712 specification by using the Openzepelin's EIP712.

The Openzeppelin's EIP712 dynamically gets the domain seperation ensuring that the chainid is same before using the cached value else the new Domain separator is hashed based on chainId

// Openzeppelin's implemenation of EIP712.
constructor(string memory name, string memory version) {
        _name = name.toShortStringWithFallback(_nameFallback);
        _version = version.toShortStringWithFallback(_versionFallback);
        _hashedName = keccak256(bytes(name));
        _hashedVersion = keccak256(bytes(version));

        _cachedChainId = block.chainid;
        _cachedDomainSeparator = _buildDomainSeparator();
        _cachedThis = address(this);
    }

    /**
     * @dev Returns the domain separator for the current chain.
     */
    function _domainSeparatorV4() internal view returns (bytes32) {
        if (address(this) == _cachedThis && block.chainid == _cachedChainId) {
            return _cachedDomainSeparator;
        } else {
            return _buildDomainSeparator();
        }
    }

    function _buildDomainSeparator() private view returns (bytes32) {
        return keccak256(abi.encode(TYPE_HASH, _hashedName, _hashedVersion, block.chainid, address(this)));
    }

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #60

[c4-judge]

0xA5DF marked the issue as unsatisfactory: Out of scope