Skip to content

Conversation

@ericglau
Copy link
Member

@ericglau ericglau commented Aug 28, 2025

Adds a Confidential tab that implements a confidential fungible token, using OpenZeppelin Confidential Contracts.

Includes:

  • @openzeppelin/wizard-confidential package, UI, AI assistant, MCP tools.

@ericglau
Copy link
Member Author

@SocketSecurity ignore npm/@fhevm/[email protected]
Package appears to be for mocks

@CoveMB
Copy link
Contributor

CoveMB commented Sep 24, 2025

Screenshot 2025-09-24 at 5 21 25 PM

Not sure if we can make AI description more clear as it already mention confidential + erc-20

Also wondering in tab using "confidential" vs "Zama" ?

"openai": "https://esm.sh/[email protected]"
"openai": "https://esm.sh/[email protected]",
"jszip": "https://esm.sh/[email protected]",
"@openzeppelin/wizard": "../../node_modules/@openzeppelin/wizard/dist/",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are those wizard import needed? (could we import from "../core.."?

Copy link
Member Author

@ericglau ericglau Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are needed because there's cross-package imports between the Solidity and Confidential core packages, some of which are used for the types in the AI function definitions. Simplified to use ../core

ericglau added a commit to ericglau/contracts-wizard that referenced this pull request Sep 25, 2025
@ericglau ericglau requested a review from CoveMB October 16, 2025 19:15
@ericglau
Copy link
Member Author

@SocketSecurity ignore npm/[email protected]
Appears to be false positive

Copy link
Contributor

@CoveMB CoveMB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

I tried the security review from MCP unittest which resulted in the following (are any relevant to the case?)

Recommendations

New confidential tooling is structurally sound, but one concurrency bug can leak per-contract metadata, and newly introduced pre-release
FHE dependencies deserve extra due diligence.

Risk Level Low

UnvalidatedNetworkConfig
location: packages/core/confidential/src/confidentialFungible.ts:45
type: input validation / configuration hardening
description: addNetworkConfig silently does nothing if networkConfig falls outside the hard-coded list. Typed callers are protected,
but any untyped consumer (custom JS integration, AI tooling bugs) can emit a contract missing the required FHE network parent, leaving
deployments misconfigured and functions reverting at runtime.
recommendation: Throw an OptionsError when the value is not one of the supported configs so invalid input fails fast.

PreReleaseFHEDependencies
location: packages/core/confidential/package.json:27, packages/core/confidential/src/environments/hardhat/package.json:13
type: dependency risk
description: The new confidential package and Hardhat template depend on pre-release FHE artifacts (@fhevm/[email protected], @fhevm/
[email protected], @zama-fhe/[email protected]). These builds have limited hardening history, so upstream changes or latent
vulnerabilities pose elevated supply-chain risk.
recommendation: Perform targeted security review of these transitive dependencies, pin to vetted versions, and monitor for advisories
before promoting the wizard to production use.

--

Suggested next steps:

  1. add runtime validation for networkConfig;
  2. schedule a dependency risk assessment (or version pinning) for the new FHE toolchain.

//--

Tests Recommendations

Risk Level High

printContractVersioned
location: packages/core/confidential/src/print-versioned.ts
change type: added
reason: New Remix printer rewrites @openzeppelin/* and @fhevm/* import paths using pinned versions; a regression would generate broken
Remix projects (external tool integration).
suggested tests:

  • Build a ConfidentialFungible contract with wrappable and votes enabled, run printContractVersioned, and assert the output replaces
    @openzeppelin/contracts, @openzeppelin/confidential-contracts, and @fhevm/solidity with the pinned versions from contract-version-pins.
  • Feed a contract that only imports third-party modules outside those prefixes and assert the printer leaves those paths untouched to
    prevent over-eager rewrites.

Risk Level Medium

injectHyperlinks
location: packages/ui/src/confidential/inject-hyperlinks.ts
change type: added
reason: Regex-based HTML injection now covers confidential and FHEVM imports; mistakes would surface as broken or unsafe links in the code
viewer (HTML sanitization risk).
suggested tests:

  • Unit test the helper with sample highlighted code containing each supported import (@openzeppelin/contracts, @openzeppelin/confidential-
    contracts, @fhevm/solidity) and assert the generated URLs include the correct pinned versions.
  • Add a negative case using a path with ../ segments to confirm the guard still prevents hyperlink injection for disallowed patterns.

evaluateSelection
location: packages/ui/src/main.ts:66
change type: modified
reason: Added the confidential branch that gates whether the new wizard loads or shows the incompatibility banner; misrouting would block
the feature.
suggested tests:

  • Unit test evaluateSelection('confidential', undefined) and evaluateSelection('confidential', incompatibleVersion) to verify it
    respectively returns the ConfidentialApp path or an incompatible response with the expected semver hint.
  • Integration-style test stubbing new ConfidentialApp to ensure the dispatcher is invoked when the selection reports compatible: true for
    the confidential language.

}

/**
* Check for potential premint overflow assuming the user's contract has decimals() = 6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be worth adding a comment in the contract that decimals must be 6 max? (I see ConfidentialFungibleTokenERC20Wrapper as maxDecimal of 6 but in case user override it)

Copy link
Member Author

@ericglau ericglau Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the user can possibly override _maxDecimals to be >6. Using a large decimals just reduces the logical max supply, and this is significant because the type euint64 has a smaller max value than typical uint256 of regular contracts.

OTOH, our checks for fields like premint might be a little restrictive if the user has less decimals(), since they could have higher supply. But I think it is fine since that's the default, and they could just manually edit the premint amount in the resulting contract if they know what they're doing.

"@upstash/redis": "https://esm.sh/@upstash/[email protected]",
"openai": "https://esm.sh/[email protected]"
"openai": "https://esm.sh/[email protected]",
"jszip": "https://esm.sh/[email protected]",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is jszip needed on deno side?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If removed, yarn type:check:api fails:

error: Relative import path "jszip" not prefixed with / or ./ or ../ and not in import map from "file:///Users/eric/git/contracts-wizard/packages/core/solidity/dist/zip-hardhat.d.ts"
    at file:///Users/eric/git/contracts-wizard/packages/core/solidity/dist/zip-hardhat.d.ts:1:19

It might be needed because it's a transitive dependency of the line below: "@openzeppelin/wizard/dist/": "../core/solidity/dist/"

@ericglau
Copy link
Member Author

@SocketSecurity ignore-all
These are AI detected anomalies and look like false positives.

@ericglau
Copy link
Member Author

security review from MCP unittest

  • UnvalidatedNetworkConfig: fixed
  • PreReleaseFHEDependencies: this is the current state of the supported upstream dependencies. Will be improved when they are supported in newer versions of Confidential Contracts.
  • printContractVersioned: fixed
  • injectHyperlinks, evaluateSelection: unnecessary, since any issues should be quite apparent in the UI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants