Skip to content

Fix: Harden AI-CNS governance contracts against unauthorized actions and reentrancy - #11

Open
mertcano wants to merge 3 commits into
Consensys:mainfrom
mertcano:mertcano-patch-1
Open

mertcano wants to merge 3 commits into
Consensys:mainfrom
mertcano:mertcano-patch-1

Conversation

@mertcano

@mertcano mertcano commented Aug 20, 2026 •

Copy link
Copy Markdown

Description

This PR hardens the core governance smart contracts (NetworkStateAgreement and NetworkStateInitiatives) within the ai-cns repository, addressing critical authorization, validation, and execution vulnerabilities.

Vulnerabilities & Security Defects Remediated:

  • Cryptographic Authorization & Replay (NetworkStateAgreement.sol): signAgreement now correctly hashes the contract address, chain ID, caller, profile, agent nature, and constitution, recovering the signer via OpenZeppelin's ECDSA. This strictly requires recoveredSigner == msg.sender, preventing attackers from submitting arbitrary signature bytes.
  • Checks-Effects-Interactions (NetworkStateAgreement.sol): Agreement state is committed only after the treasury transfer and cross-contract credit allocation succeed, mitigating reentrancy risks alongside the nonReentrant modifier.
  • Unauthorized Credit Issuance & Regeneration (NetworkStateInitiatives.sol): updateUserCredits is now restricted by the onlyCreditManager modifier. The implicit credit refill mechanism was removed from the voting path, and exhausted credits can no longer be regenerated via repeated calls.
  • Voting Without Agreement Membership (NetworkStateInitiatives.sol): Voting paths (upvote and downvote) now strictly invoke _requireAgreement(), meaning unverified users can no longer mutate vote state.
  • Initiator Spoofing (NetworkStateInitiatives.sol): createInitiatives now strictly requires _ideator == msg.sender, keeping proposal ownership aligned with the transaction signer.
  • Silent Acceptance of Unknown IDs (NetworkStateInitiatives.sol): Funding, voting, status, and team-management paths now use _requireInitiative(), which correctly reverts if an initiative ID is not found, preventing silent state corruption or trapped ether.
  • Status and Team Authorization Drift (NetworkStateInitiatives.sol): Initiative status updates are restricted strictly to onlyOwner. Team membership changes now explicitly check that the caller is the owner, ideator, or instigator.
  • Unsafe Ether Transfer (NetworkStateInitiatives.sol): Replaced fixed-gas transfer calls with checked low-level call{value: ...}("") under nonReentrant guards for safe treasury forwarding and withdrawals.

Note

High Risk
Touches core on-chain auth, credit issuance, voting, and ETH transfers. Incorrect signature, instigator, or reentrancy behavior could lock funds or let unauthorized users vote.

Overview
Hardens the two governance contracts so agreement signing, credits, voting, and funding can no longer be spoofed or silently fail.

NetworkStateAgreement now verifies ECDSA signatures over contract, chain, caller, profile, agent, and constitution hash. Constructor rejects zero addresses. Ether is forwarded and credits are granted before recording hasAgreed. Adds a public hasAgreed view for the initiatives contract.

NetworkStateInitiatives wires an agreement contract (setAgreementContract) and requires a signed agreement before upvote/downvote. Credits can only be set by owner or the agreement contract; the old auto-refill on vote is gone. Creating an initiative requires _ideator == msg.sender. Unknown initiative IDs revert. Status updates are owner-only. Team edits are limited to owner/ideator/instigator. Instigator is assigned to the ideator after a +5 upvote margin instead of during status change.

Withdrawals use checked call plus nonReentrant. Initiative IDs include a per-sender nonce instead of block entropy.

Reviewed by Cursor Bugbot for commit 34fb235. Bugbot is set up for automated code reviews on this repo. Configure here.

…and reentrancy

### Description
This PR hardens the core governance smart contracts (`NetworkStateAgreement` and `NetworkStateInitiatives`) within the `ai-cns` repository, addressing critical authorization, validation, and execution vulnerabilities.

**Vulnerabilities & Security Defects Remediated:**
* **Cryptographic Authorization & Replay (`NetworkStateAgreement.sol`):** `signAgreement` now correctly hashes the contract address, chain ID, caller, profile, agent nature, and constitution, recovering the signer via OpenZeppelin's `ECDSA`. This strictly requires `recoveredSigner == msg.sender`, preventing attackers from submitting arbitrary signature bytes.
* **Checks-Effects-Interactions (`NetworkStateAgreement.sol`):** Agreement state is committed only after the treasury transfer and cross-contract credit allocation succeed, mitigating reentrancy risks alongside the `nonReentrant` modifier.
* **Unauthorized Credit Issuance & Regeneration (`NetworkStateInitiatives.sol`):** `updateUserCredits` is now restricted by the `onlyCreditManager` modifier. The implicit credit refill mechanism was removed from the voting path, and exhausted credits can no longer be regenerated via repeated calls.
* **Voting Without Agreement Membership (`NetworkStateInitiatives.sol`):** Voting paths (`upvote` and `downvote`) now strictly invoke `_requireAgreement()`, meaning unverified users can no longer mutate vote state.
* **Initiator Spoofing (`NetworkStateInitiatives.sol`):** `createInitiatives` now strictly requires `_ideator == msg.sender`, keeping proposal ownership aligned with the transaction signer.
* **Silent Acceptance of Unknown IDs (`NetworkStateInitiatives.sol`):** Funding, voting, status, and team-management paths now use `_requireInitiative()`, which correctly reverts if an initiative ID is not found, preventing silent state corruption or trapped ether.
* **Status and Team Authorization Drift (`NetworkStateInitiatives.sol`):** Initiative status updates are restricted strictly to `onlyOwner`. Team membership changes now explicitly check that the caller is the owner, ideator, or instigator.
* **Unsafe Ether Transfer (`NetworkStateInitiatives.sol`):** Replaced fixed-gas `transfer` calls with checked low-level `call{value: ...}("")` under `nonReentrant` guards for safe treasury forwarding and withdrawals.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 54a9e25. Configure here.

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.

1 participant