Infinex A-14

In the TransferModule, when encoding the requestHash to verify and validate the user’s and cosign’s signature operations, the requiresCoSigning() modifier encodes the _request parameter directly with the typehash and message signature:

modifier requiresCoSigning(
    TransferRequest calldata _request,
    bytes calldata _sudoKeySignature,
    bytes calldata _platformKeySignature
) {
    if (!Cosigning._isValidNonce(_request.nonce)) revert InvalidNonce();

    **bytes32 requestHash = EIP712._hashTypedDataV4(keccak256(abi.encode(Cosigning._TRANSFER_REQUEST_TYPEHASH, msg.sig, _request)));**
    SecurityUtils._validateCoSignature(requestHash, _sudoKeySignature, _platformKeySignature);

    Cosigning._consumeNonce(_request.nonce);

    _;
}

Reference: TransferModule.sol#L48-61

However, the TransferRequest custom data type has two array parameters (amounts and tokenIds) and one dynamic type member (data), which should be handled separately as per EIP712 specification:

The dynamic values bytes and string are encoded as a keccak256 hash of their contents.

The array values are encoded as the keccak256 hash of the concatenated encodeData of their contents (i.e. the encoding of SomeType[5] is identical to that of a struct containing five members of type SomeType).

Remediations to Consider:

Consider implementing the encoding and hashing of tokenIds, amounts and data as per EIP712 specification:

keccak256(abi.encode(
    Cosigning._TRANSFER_REQUEST_TYPEHASH,
    msg.sig,
    _request.nonce,
    _request.token,
    _request.destination,
    _request.amount,
    _request.tokenId,
    keccak256(abi.encodePacked(_request.tokenIds)), // Array handling
    keccak256(abi.encodePacked(_request.amounts)),  // Array handling
    keccak256(request.data),                        // Dynamic bytes handling
    request.nonStandardIndex
));

The transferERC721() and transferERC721ABatch() functions in the TransferModule contract do not include a data parameter when calling the ERC721.safeTransferFrom() and ERC721ABatchTransferable.safeBatchTransferFrom() functions. This means the onERC721Received() hook will always receive empty data, potentially limiting hook functionality for contracts that rely on this parameter.

function _transferERC721(TransferRequest calldata _request) internal {
    ...

>>  IERC721(_request.token).safeTransferFrom(address(this), _request.destination, _request.tokenId);
}

function _transferERC721ABatch(TransferRequest calldata _request) internal {
    ...

>>  IERC721ABatchTransferable(_request.token).safeBatchTransferFrom(address(this), _request.destination, _request.tokenIds);
}

Reference: TransferModule.sol#L219-L238

Remediations to Consider:

Consider using the data field in the TransferRequest struct for ERC721 transfers, similar to how it's handled for ERC1155 transfers.

Ownable.sol allows for ownership of contact that inherit, allowing functions that use the onlyOwner modifier to be gated to only be callable by the set owner. Ownership can allow be transferred via a nomination and acceptance process via nominateNewOwner() and acceptOwnership(), with the nominee able to renounce a nomination via renounceNomination(). However, each permission check uses msg.sender rather than _msgSender(). This means that contracts that utilize a ERC2771 trusted forwarder, which many infinex contracts do, will not be able to use this forwarder to make owner permissioned calls, nor nominate, renounce, or accept ownership.

Remediations to Consider

Use _msgSender() and inherit Openzeppelin’s Context.sol, overriding it in contracts that use a trusted forwarder.

Ownable.sol offers no way to renounce ownership of the contract. Currently the owner can only nominate a new owner, which cannot be the zero address, and is only set once the nominee accepts, meaning the contract will always have an owner. Renouncing ownership is sometimes beneficial to prevent permissioned functions from being callable, reducing or eliminating the trust assumptions of the contract.

Remediations to Consider

Add the ability for the owner to renounce their own ownership, leaving the contract without an owner.

Consider including the tokenIds transferred in the event parameters, similar to the ERC1155BatchTransferred event.

When interacting with non-standard NFTs, the newly added functions recoverNonStandardNFT() and transferNonStandardToken() use a zero value in the _nonStandardIndex to transfer CryptoPunks. More specifically, in the WithdrawUtil contract:

function _transferNonStandardNFT(address _transferAddress, address _token, uint256 _tokenId, uint256 _nonStandardIndex) internal {
    emit TransferNonStandardNFTExecuted(_token, _transferAddress, _tokenId, _nonStandardIndex);
    **if (_nonStandardIndex == 0) {**
        ICryptoPunks(_token).transferPunk(_transferAddress, _tokenId);
    } else if (_nonStandardIndex == 1) {
        IEtherRocks(_token).giftRock(_tokenId, _transferAddress);
    } else {
        revert Error.UnsupportedNonStandardIndex();
    }
}

Reference: WithdrawUtil.sol#L151-160

Although the msg.sig is checked against the signed message, consider using non-zero values for each supported non-standard NFT.

There are currently two different variations of the msg.sig sanity checks to ensure the calldata payload matches the request:

• requiresSudoKey() utilizes the _request._selector and reverts if not valid.

modifier requiresSudoKey(RequestTypes.Request calldata _request, bytes calldata _signature) {
    **if (_request._selector != msg.sig) {
        revert Error.InvalidRequest();
    }**
    bytes32 messageHash = EIP712._hashTypedDataV4(keccak256(abi.encode(SecurityKeys._SIGNATURE_REQUEST_TYPEHASH, _request)));
        ...
}

Reference: SecurityModifiers.sol#L34-49

• requiresCoSigning() utilizes the msg.sig, encodes it in the requestHash and verifies the provided signature for both TransferRequest and RecoveryRequest.

modifier requiresCoSigning(
    TransferRequest calldata _request,
    bytes calldata _sudoKeySignature,
    bytes calldata _platformKeySignature
) {
    ...

    **bytes32 requestHash = EIP712._hashTypedDataV4(keccak256(abi.encode(Cosigning._TRANSFER_REQUEST_TYPEHASH, msg.sig, _request)));**

    ...
}

Reference: TransferModule.sol#L48-61

Consider adding the _selector member to all requests to keep the code consistent with the previous implementation. Additionally, the requestHash and signed data would be consistent with the typehash contents(_TRANSFER_REQUEST_TYPEHASH and _RECOVERY_REQUEST_TYPEHASH).

In the TransferModule contract, the transfer functions implement inconsistent checks on the request parameters, specifically on the destination and token address.

• _transferERC20 lacks checks for both addresses.
• _transferERC1155 and _transferERC1155Batch functions lack checks for address(0) for both addresses.

Consider implementing these checks to be consistent across all transfer functions.

The transfer() function in the NonStandardToken library duplicates the non-standard token transfer logic found in the _transferNonStandardNFT() function in the WithdrawUtil library. Currently, only two non-standard tokens (CryptoPunks and EtherRocks) are supported. When adding support for new non-standard tokens, both functions must be modified, which creates maintenance overhead and increases the risk of inconsistencies.

Consider consolidating the non-standard token transfer logic into a single function that both libraries can reuse.

OwnedMockUpgreadable.sol is still referencing Synthetix’s UUPSImplementation instead of the local version, see here.

Additionally, the file and contract name is spelled incorrectly. Consider renaming them to OwnedMockUpgradable.

The infinex protocol used Synthetix based proxies and ownership for deployed contract which utilized its own storage slots for implementation and owner. These new proxies will now be deployed using differing storage slots for these values, with implementation and owner. This means that deployed contracts using the prior slots will be incompatible with contracts using this new version. If attempting to upgrade from the synthetix version to the new infinex the call will currently fail during the simulation. However, upgrading should be handled with extra caution, if incomparable upgrades are possible in the future they could cause ownership to be removed unintentionally. Since there are now two incompatible storage versions it may require writing two variations of each contract for an update. It may be wise to instead create a method to migrate both the implementation and owner slots to the new version before or during an upgrade to allow for contracts to use the same codebase and remove the synthetix dependancy.