feat: implement identity recovery and backup wallet logic#75
feat: implement identity recovery and backup wallet logic#75dhruvi-16-me wants to merge 1 commit into
Conversation
WalkthroughThis pull request implements a secure backup wallet recovery system for identity tokens. It introduces a timelock mechanism for backup wallet updates, allows compromise flagging and identity recovery workflows, and adds endorsement revocation functionality. The token transfer logic is modified to permit operations during explicit recovery scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Owner
participant IdentityToken as IdentityToken<br/>(Contract)
participant Timelock as 7-Day<br/>Timelock
Owner->>IdentityToken: initiateBackupUpdate(tokenId, newBackup)
activate IdentityToken
IdentityToken->>IdentityToken: Set pendingBackupWallet<br/>Set backupUnlockTime
IdentityToken->>Owner: emit BackupUpdateInitiated
deactivate IdentityToken
Owner->>Timelock: Wait 7 days
Timelock->>Owner: Timelock expires
Owner->>IdentityToken: finalizeBackupUpdate(tokenId)
activate IdentityToken
IdentityToken->>IdentityToken: Verify timelock passed
IdentityToken->>IdentityToken: Commit pendingBackupWallet<br/>Clear pending fields
IdentityToken->>Owner: emit BackupUpdated
deactivate IdentityToken
sequenceDiagram
participant Owner as Owner/<br/>BackupWallet
participant IdentityToken as IdentityToken<br/>(Contract)
participant NewOwner
Owner->>IdentityToken: flagCompromised(tokenId)
activate IdentityToken
IdentityToken->>IdentityToken: Set isCompromised = true
IdentityToken->>Owner: emit IdentityCompromised
deactivate IdentityToken
Note over IdentityToken: Token operations frozen<br/>until recovery
BackupWallet->>IdentityToken: recoverIdentity(tokenId, newOwner)
activate IdentityToken
IdentityToken->>IdentityToken: Verify caller is backupWallet
IdentityToken->>IdentityToken: Transfer ERC-721 ownership<br/>Update ownerToTokenId mapping
IdentityToken->>IdentityToken: Set isCompromised = false
IdentityToken->>NewOwner: emit IdentityRecovered
deactivate IdentityToken
Note over IdentityToken: NewOwner can now<br/>modify attributes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/IdentityToken.t.sol (1)
1-828:⚠️ Potential issue | 🟠 MajorMissing test coverage for
revokeEndorsement.The new
revokeEndorsementfunction inIdentityToken.solhas no corresponding tests. Per coding guidelines, security-sensitive logic changes require adequate test coverage. The following scenarios should be tested:
- Successful revocation by endorser
- Event emission (
EndorsementRevoked)- Revert when index is out of bounds (
IndexOutOfBounds)- Revert when caller is not the endorser (
NotEndorser)- Revert when endorsement is already revoked (
AlreadyRevoked)- Verify
revokedAttimestamp is set correctly- Verify revoked endorsement no longer counts as active in
isVerifiedDo you want me to generate the test cases for
revokeEndorsement?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/IdentityToken.t.sol` around lines 1 - 828, Add unit tests for revokeEndorsement covering: (1) successful revocation by the endorser — mint two tokens, call endorse(endorserId, recipientId,...), then vm.prank(endorser) call revokeEndorsement(recipientId, index) and assert endorsements(recipientId, index).revokedAt > 0 and isVerified(recipientId) updates accordingly; (2) event emission — wrap the revoke call with vm.expectEmit and assert Events.EndorsementRevoked(recipientId, endorserId, index); (3) index out of bounds revert — call revokeEndorsement with an invalid index and vm.expectRevert(Errors.IndexOutOfBounds.selector); (4) unauthorized caller revert — have a non-endorser call revokeEndorsement and vm.expectRevert(Errors.NotEndorser.selector); and (5) already revoked revert — revoke once, then attempt to revoke again and vm.expectRevert(Errors.AlreadyRevoked.selector). Use identityToken.endorse, identityToken.revokeEndorsement, the endorsements(recipientId, index) accessor, vm.prank for callers, vm.warp if checking timestamp comparisons, and Events.EndorsementRevoked / Errors.* selectors to locate assertions.
🤖 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`:
- Around line 259-261: The recovery pattern using the _recovering flag should be
hardened by adding OpenZeppelin's ReentrancyGuard: import
"@openzeppelin/contracts/security/ReentrancyGuard.sol", have the contract
inherit ReentrancyGuard, and add the nonReentrant modifier to the recovery
function that currently toggles _recovering and calls _transfer (the function
that sets _recovering = true; _transfer(currentOwner, newOwner, tokenId);
_recovering = false). This preserves the existing flag logic while providing
defense-in-depth against future reentrancy risks.
- Around line 206-214: In initiateBackupUpdate, prevent the owner from setting
their own address as the backup by validating that newBackup is not equal to the
token owner; retrieve the owner (e.g. via ownerOf(tokenId) or currentTokenOwner
lookup used by onlyTokenOwner) and add a require/revert if newBackup == owner,
then proceed to set pendingBackupWallet and backupUnlockTime and emit
Events.BackupUpdateInitiated; reference the function initiateBackupUpdate and
the DataTypes.IdentityState.pendingBackupWallet field when making this change.
- Around line 240-247: Add a guard in flagCompromised to avoid redundant work
and duplicate events by checking identityStates[tokenId].isCompromised before
setting it; if already true, revert with a descriptive error (e.g.,
Errors.AlreadyCompromised()) or make the call a no-op, then only set
state.isCompromised = true and emit Events.IdentityCompromised(tokenId) when the
token was not previously compromised; keep existing ownership check (ownerOf and
state.backupWallet) as-is.
- Around line 277-292: Add unit tests covering revokeEndorsement: implement
tests for (1) successful revoke — set up an endorsement (create tokens/users,
set ownerToTokenId for endorser, add endorsement to
endorsements[targetTokenId]), call revokeEndorsement and assert
endorsements[targetTokenId][index].revokedAt is nonzero and
Events.EndorsementRevoked was emitted with (endorserTokenId, targetTokenId,
index); (2) IndexOutOfBounds — call revokeEndorsement with an out-of-range index
and assert it reverts with Errors.IndexOutOfBounds; (3) NotEndorser — call as a
caller without ownerToTokenId or with mismatched endorserTokenId and assert
revert with Errors.NotEndorser; (4) AlreadyRevoked — revoke once then call again
and assert revert with Errors.AlreadyRevoked. Use the contract functions/state
names from the diff (revokeEndorsement, endorsements, ownerToTokenId,
Events.EndorsementRevoked, Errors.*) to locate and exercise the logic.
- Around line 254-266: Add a zero-address check in recoverIdentity to prevent
recovering an identity to address(0): inside the recoverIdentity function
(alongside the existing balanceOf and onlyBackupWallet checks) require newOwner
!= address(0) and revert with a defined error from Errors.sol; either add a new
InvalidRecipient error to Errors.sol and use that, or reuse the existing
TargetInvalid error (update the revert call in recoverIdentity accordingly).
Ensure the change references recoverIdentity and Errors.sol only, and keep the
remainder of the function logic (setting _recovering, _transfer, clearing
isCompromised, and emitting Events.IdentityRecovered) unchanged.
---
Outside diff comments:
In `@test/IdentityToken.t.sol`:
- Around line 1-828: Add unit tests for revokeEndorsement covering: (1)
successful revocation by the endorser — mint two tokens, call
endorse(endorserId, recipientId,...), then vm.prank(endorser) call
revokeEndorsement(recipientId, index) and assert endorsements(recipientId,
index).revokedAt > 0 and isVerified(recipientId) updates accordingly; (2) event
emission — wrap the revoke call with vm.expectEmit and assert
Events.EndorsementRevoked(recipientId, endorserId, index); (3) index out of
bounds revert — call revokeEndorsement with an invalid index and
vm.expectRevert(Errors.IndexOutOfBounds.selector); (4) unauthorized caller
revert — have a non-endorser call revokeEndorsement and
vm.expectRevert(Errors.NotEndorser.selector); and (5) already revoked revert —
revoke once, then attempt to revoke again and
vm.expectRevert(Errors.AlreadyRevoked.selector). Use identityToken.endorse,
identityToken.revokeEndorsement, the endorsements(recipientId, index) accessor,
vm.prank for callers, vm.warp if checking timestamp comparisons, and
Events.EndorsementRevoked / Errors.* selectors to locate assertions.
🪄 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: 40372957-75ac-48f7-8e22-5ce4e7dc1c77
📒 Files selected for processing (2)
src/IdentityToken.soltest/IdentityToken.t.sol
There was a problem hiding this comment.
Pull request overview
Implements an identity backup wallet + recovery mechanism for IdentityToken to support account loss/compromise scenarios while keeping the token generally non-transferable.
Changes:
- Added timelocked backup wallet update flow (initiate/finalize) plus compromise flagging.
- Added backup-wallet-controlled identity recovery path and updated transfer restrictions to allow recovery transfers.
- Added Foundry tests covering backup wallet management, compromise flagging, and recovery flows.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/IdentityToken.sol |
Adds backup wallet timelock logic, compromised/recovery flows, and endorsement revocation; updates _update transfer restrictions for recovery. |
test/IdentityToken.t.sol |
Adds test cases for backup wallet state transitions, access control, compromise freezing, and recovery behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _; | ||
| } | ||
|
|
||
| modifier onlyBackupWallet(uint256 tokenId) { |
There was a problem hiding this comment.
onlyBackupWallet checks identityStates[tokenId].backupWallet before verifying the token exists. For a nonexistent tokenId, recoverIdentity will revert with NotBackupWallet rather than the standard ERC721 nonexistent-token revert, which is misleading and can break callers relying on that behavior. Consider validating ownership/existence first (e.g., _requireOwned(tokenId) or ownerOf(tokenId) inside the modifier) before checking the backup wallet.
| modifier onlyBackupWallet(uint256 tokenId) { | |
| modifier onlyBackupWallet(uint256 tokenId) { | |
| _requireOwned(tokenId); |
| function recoverIdentity(uint256 tokenId, address newOwner) external onlyBackupWallet(tokenId) { | ||
| if (balanceOf(newOwner) != 0) revert Errors.AlreadyHasIdentity(); | ||
|
|
||
| address currentOwner = ownerOf(tokenId); | ||
|
|
||
| _recovering = true; | ||
| _transfer(currentOwner, newOwner, tokenId); | ||
| _recovering = false; |
There was a problem hiding this comment.
recoverIdentity currently allows a backup wallet to transfer the identity even when it is not marked compromised. That creates a general-purpose transfer path (owner can set a backup wallet, wait the timelock, and have it “recover” to any address), which weakens the intended soulbound / non-transferable guarantees. If recovery is meant to be an emergency-only path, require identityStates[tokenId].isCompromised == true (or another explicit recovery-initiated flag) before allowing the transfer.
| * may call this. Clears the isCompromised flag post-transfer. | ||
| */ | ||
| function recoverIdentity(uint256 tokenId, address newOwner) external onlyBackupWallet(tokenId) { | ||
| if (balanceOf(newOwner) != 0) revert Errors.AlreadyHasIdentity(); | ||
|
|
||
| address currentOwner = ownerOf(tokenId); | ||
|
|
||
| _recovering = true; | ||
| _transfer(currentOwner, newOwner, tokenId); | ||
| _recovering = false; | ||
|
|
||
| identityStates[tokenId].isCompromised = false; |
There was a problem hiding this comment.
After recoverIdentity, the previous backupWallet remains set. This means the same backup wallet can immediately “recover” again (and the new owner can only change the backup via a 7-day timelock), which is a strong and possibly unintended power over the recovered identity. Consider clearing backupWallet/pending fields on recovery, or updating the backup wallet as part of the recovery flow so the new owner isn’t stuck with the prior guardian.
| * may call this. Clears the isCompromised flag post-transfer. | |
| */ | |
| function recoverIdentity(uint256 tokenId, address newOwner) external onlyBackupWallet(tokenId) { | |
| if (balanceOf(newOwner) != 0) revert Errors.AlreadyHasIdentity(); | |
| address currentOwner = ownerOf(tokenId); | |
| _recovering = true; | |
| _transfer(currentOwner, newOwner, tokenId); | |
| _recovering = false; | |
| identityStates[tokenId].isCompromised = false; | |
| * may call this. Clears the isCompromised flag post-transfer and | |
| * removes the existing backup wallet to avoid repeated recoveries | |
| * by the previous guardian. | |
| */ | |
| function recoverIdentity(uint256 tokenId, address newOwner) external onlyBackupWallet(tokenId) { | |
| if (balanceOf(newOwner) != 0) revert Errors.AlreadyHasIdentity(); | |
| address currentOwner = ownerOf(tokenId); | |
| DataTypes.IdentityState storage state = identityStates[tokenId]; | |
| _recovering = true; | |
| _transfer(currentOwner, newOwner, tokenId); | |
| _recovering = false; | |
| state.isCompromised = false; | |
| state.backupWallet = address(0); |
| DataTypes.Endorsement storage e = list[index]; | ||
|
|
||
| uint256 callerTokenId = ownerToTokenId[msg.sender]; | ||
| if (callerTokenId == 0 || e.endorserTokenId != callerTokenId) revert Errors.NotEndorser(); |
There was a problem hiding this comment.
flagCompromised’s docstring says it freezes “all attribute and endorsement operations,” but revokeEndorsement has no notCompromised check for the caller’s identity. Either add a compromised-state guard for revocations (e.g., require the caller’s tokenId is not compromised) or adjust the documentation/expectations so it’s clear revocation remains allowed while compromised.
| if (callerTokenId == 0 || e.endorserTokenId != callerTokenId) revert Errors.NotEndorser(); | |
| if (callerTokenId == 0 || e.endorserTokenId != callerTokenId) revert Errors.NotEndorser(); | |
| if (identityStates[callerTokenId].isCompromised) revert Errors.CompromisedIdentity(); |
| function revokeEndorsement(uint256 targetTokenId, uint256 index) external { | ||
| DataTypes.Endorsement[] storage list = endorsements[targetTokenId]; | ||
|
|
||
| if (index >= list.length) revert Errors.IndexOutOfBounds(); | ||
|
|
||
| DataTypes.Endorsement storage e = list[index]; | ||
|
|
||
| uint256 callerTokenId = ownerToTokenId[msg.sender]; | ||
| if (callerTokenId == 0 || e.endorserTokenId != callerTokenId) revert Errors.NotEndorser(); | ||
|
|
||
| if (e.revokedAt != 0) revert Errors.AlreadyRevoked(); | ||
|
|
||
| e.revokedAt = block.timestamp; | ||
|
|
||
| emit Events.EndorsementRevoked(e.endorserTokenId, targetTokenId, index); | ||
| } |
There was a problem hiding this comment.
revokeEndorsement is newly introduced behavior but there are no tests covering the happy path (revoking an active endorsement), the authorization checks (NotEndorser), and the double-revoke case (AlreadyRevoked). Adding targeted tests would help ensure revocation semantics stay correct and don’t regress.
|
@dhruvi-16-me please resolve the issues pointed out by copilot and coderabbit above. |
Addressed Issues:
Fixes #48
Screenshots/Recordings:
Additional Notes:
This PR implements a secure identity recovery and backup wallet mechanism for
IdentityToken, enabling users to recover ownership of their identity if their primary wallet is lost or compromised.The implementation introduces a timelocked backup wallet update flow, compromise flagging, and a controlled recovery process while preserving the soulbound (non-transferable) nature of the token.
🔧 Changes Made
Backup Wallet Management
Added support for assigning a
backupWalletwith a 7-day timelockImplemented:
initiateBackupUpdate(uint256 tokenId, address newBackup)— stores pending backup and unlock timestampfinalizeBackupUpdate(uint256 tokenId)— finalizes backup assignment after timelockEmits:
BackupUpdateInitiatedBackupUpdatedCompromise Flagging
Added
flagCompromised(uint256 tokenId)callable by:Sets
isCompromised = true, restricting protocol actions via existing guardsEmits
IdentityCompromisedIdentity Recovery
Implemented
recoverIdentity(uint256 tokenId, address newOwner):onlyBackupWalletnewOwnerownerToTokenIdmappingisCompromisedtofalseEmits
IdentityRecoveredTransfer Safety
Updated
_updateto enforce non-transferability:Introduced
_recoveringflag to safely allow transfers only during recoveryTesting
Added 14 comprehensive test cases in
IdentityToken.t.solcovering:Backup Wallet Management
Compromise Flagging
Identity Recovery
All tests pass and cover both expected behavior and edge cases.
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.
Checklist
Summary by CodeRabbit
New Features
Tests