Skip to content

fix: invalidate compromised endorsers in isVerified#81

Closed
SIDDHANTCOOKIE wants to merge 2 commits into
StabilityNexus:mainfrom
SIDDHANTCOOKIE:fix/compromised-endorser-validation
Closed

fix: invalidate compromised endorsers in isVerified#81
SIDDHANTCOOKIE wants to merge 2 commits into
StabilityNexus:mainfrom
SIDDHANTCOOKIE:fix/compromised-endorser-validation

Conversation

@SIDDHANTCOOKIE

@SIDDHANTCOOKIE SIDDHANTCOOKIE commented Mar 26, 2026

Copy link
Copy Markdown
Member

Addressed Issues:

Fixes #67

Updated the active check in src/IdentityToken.sol (isVerified) to also require:
!identityStates[e.endorserTokenId].isCompromised

New logic:
bool active = e.revokedAt == 0 && (e.validUntil == 0 || e.validUntil >= block.timestamp) && !identityStates[e.endorserTokenId].isCompromised;

Tests

Added one new test in test/IdentityToken.t.sol:

  • test_IsVerified_FalseIfEndorserCompromised()

Test flow:

  1. Mint endorser + target identities
  2. Create endorsement
  3. Mark endorser as compromised via stdstore
  4. Assert isVerified(target) returns false

Screenshots/Recordings:

Screenshot 2026-03-26 193237

Additional Notes:

Security Impact

Prevents compromised identities from continuing to confer trust via existing endorsements, closing a trust-graph bypass risk.

Gas / Performance

isVerified now performs one additional storage read per endorsement candidate (identityStates[endorserTokenId].isCompromised).
This is an intentional security tradeoff; short-circuiting still avoids that read for already revoked/expired endorsements.

AI Usage Disclosure:

We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.

Check one of the checkboxes below:

  • This PR does not contain AI-generated code at all.
  • This PR contains AI-generated code. I have read the AI Usage Policy and this PR complies with this policy. I have tested the code locally and I am responsible for it.

I have used the following AI models and tools: TODO

Checklist

  • My PR addresses a single issue, fixes a single bug or makes a single improvement.
  • My code follows the project's code style and conventions
  • If applicable, I have made corresponding changes or additions to the documentation
  • If applicable, I have made corresponding changes or additions to tests
  • My changes generate no new warnings or errors
  • I have joined the Discord server and I will share a link to this PR with the project maintainers there
  • I have read the Contribution Guidelines
  • Once I submit my PR, CodeRabbit AI will automatically review it and I will address CodeRabbit's comments.
  • I have filled this PR template completely and carefully, and I understand that my PR may be closed without review otherwise.

Summary by CodeRabbit

  • Bug Fixes

    • Verification now properly validates that an endorser's identity is not compromised, in addition to checking standard endorsement validity conditions like revocation status and expiration time.
  • Tests

    • Added test coverage to verify that endorsements are correctly invalidated when the endorser's identity is marked as compromised, strengthening overall identity security validation.

@coderabbitai

coderabbitai Bot commented Mar 26, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

The isVerified() function in IdentityToken.sol now validates that an endorser's identity is not compromised in addition to checking the endorsement's revocation and expiration status. A corresponding test was added to verify this behavior.

Changes

Cohort / File(s) Summary
Core Logic Update
src/IdentityToken.sol
Modified isVerified() to check identityStates[e.endorserTokenId].isCompromised before marking an endorsement as active, preventing compromised identities from issuing valid endorsements.
Test Coverage
test/IdentityToken.t.sol
Added test_IsVerified_FalseIfEndorserCompromised() to verify that isVerified() returns false when the endorsing identity is marked compromised.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested labels

Solidity Lang

Poem

🐰 A compromise detected, trust must flee,
Check the endorser's state—are they still free?
One line added, one removed with care,
Now compromised identities won't ensnare! ✨

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding a check to invalidate endorsements from compromised endorsers in the isVerified function.
Linked Issues check ✅ Passed The code changes fully implement the requirements from issue #67: isVerified now checks identityStates[e.endorserTokenId].isCompromised, preventing compromised endorsers from conferring trust.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing issue #67: one line modification in isVerified logic and one new test validating the compromised endorser check behavior.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/IdentityToken.sol`:
- Line 245: Reformat the boolean assignment that sets the local variable active
in IdentityToken.sol (the line using e.revokedAt, e.validUntil, block.timestamp
and identityStates[e.endorserTokenId].isCompromised) to satisfy the repository
Prettier rules; either run the project's Prettier/format command or adjust
spacing/line breaks around the && and || operators so the expression is split or
spaced per style guidelines and the CI formatting error is resolved.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: a9bef3ed-a6f7-4be1-8a14-e8510d6b5f72

📥 Commits

Reviewing files that changed from the base of the PR and between 22e681b and d54eef8.

📒 Files selected for processing (2)
  • src/IdentityToken.sol
  • test/IdentityToken.t.sol

Comment thread src/IdentityToken.sol Outdated
for (uint256 i = 0; i < list.length; i++) {
DataTypes.Endorsement storage e = list[i];
bool active = e.revokedAt == 0 && (e.validUntil == 0 || e.validUntil >= block.timestamp);
bool active = e.revokedAt == 0 && (e.validUntil == 0 || e.validUntil >= block.timestamp) && !identityStates[e.endorserTokenId].isCompromised;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix CI formatting failure on this updated expression.

Line 245 is likely the Prettier violation reported by CI; please reformat this statement (or run the repo Prettier command) before merge.

💄 Suggested formatting
-            bool active = e.revokedAt == 0 && (e.validUntil == 0 || e.validUntil >= block.timestamp) && !identityStates[e.endorserTokenId].isCompromised;
+            bool active = e.revokedAt == 0 &&
+                (e.validUntil == 0 || e.validUntil >= block.timestamp) &&
+                !identityStates[e.endorserTokenId].isCompromised;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
bool active = e.revokedAt == 0 && (e.validUntil == 0 || e.validUntil >= block.timestamp) && !identityStates[e.endorserTokenId].isCompromised;
bool active = e.revokedAt == 0 &&
(e.validUntil == 0 || e.validUntil >= block.timestamp) &&
!identityStates[e.endorserTokenId].isCompromised;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/IdentityToken.sol` at line 245, Reformat the boolean assignment that sets
the local variable active in IdentityToken.sol (the line using e.revokedAt,
e.validUntil, block.timestamp and
identityStates[e.endorserTokenId].isCompromised) to satisfy the repository
Prettier rules; either run the project's Prettier/format command or adjust
spacing/line breaks around the && and || operators so the expression is split or
spaced per style guidelines and the CI formatting error is resolved.

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.

[BUG]: Endorsements from Compromised Identities Remain Valid in isVerified()

2 participants