-
-
Notifications
You must be signed in to change notification settings - Fork 17
[Bug]: Fix: enforce required identity fields (name + email/phone) #63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
cf43f80
12a9ab7
5926ba3
8f60e5d
6a0d7f8
0b6ee25
340f4d9
7c36ca1
f229460
cb415de
a926546
6c30f6d
417a1be
a192669
98dc951
4cfdb02
049f8fc
c656af9
10608be
5ea5109
42ade6b
d8590fc
aa8df60
1f50c7e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,11 @@ contract IdentityToken is ERC721, IIdentityToken { | |
| // tokenId => attribute keyHash => attribute value | ||
| mapping(uint256 => mapping(bytes32 => bytes)) public attributes; | ||
|
|
||
| // required attribute key hashes | ||
| bytes32 private constant NAME_KEY = keccak256(abi.encodePacked("name")); | ||
| bytes32 private constant EMAIL_KEY = keccak256(abi.encodePacked("email")); | ||
| bytes32 private constant PHONE_KEY = keccak256(abi.encodePacked("phone")); | ||
|
|
||
| // tokenId => array of Endorsements | ||
| mapping(uint256 => DataTypes.Endorsement[]) public endorsements; | ||
|
|
||
|
|
@@ -76,6 +81,25 @@ contract IdentityToken is ERC721, IIdentityToken { | |
| return tokenId; | ||
| } | ||
|
|
||
| /** | ||
| * @dev Adds validation to ensure required identity fields are present. | ||
| * Name is mandatory and at least one contact method (email or phone) | ||
| * must be provided. | ||
| */ | ||
| function _validateRequiredFields(uint256 tokenId) internal view { | ||
| bytes storage name = attributes[tokenId][NAME_KEY]; | ||
| bytes storage email = attributes[tokenId][EMAIL_KEY]; | ||
| bytes storage phone = attributes[tokenId][PHONE_KEY]; | ||
|
|
||
| // Name is mandatory | ||
| if (name.length == 0) revert Errors.MissingName(); | ||
|
|
||
| // At least one contact method required | ||
| if (email.length == 0 && phone.length == 0) { | ||
| revert Errors.MissingContact(); | ||
| } | ||
| } | ||
|
Comment on lines
+89
to
+101
|
||
|
|
||
| /** | ||
| * @dev Sets a metadata attribute (e.g., name, social link) for an identity. | ||
| */ | ||
|
|
@@ -84,7 +108,14 @@ contract IdentityToken is ERC721, IIdentityToken { | |
| string calldata key, | ||
| bytes calldata value | ||
| ) external onlyTokenOwner(tokenId) notCompromised(tokenId) { | ||
| bytes32 keyHash = keccak256(abi.encodePacked(key)); | ||
|
|
||
| _setAttribute(tokenId, key, value); | ||
|
|
||
| // skip validation if the updated attribute is one of the required fields, since | ||
| if (keyHash != NAME_KEY && keyHash != EMAIL_KEY && keyHash != PHONE_KEY) { | ||
| _validateRequiredFields(tokenId); | ||
| } | ||
|
Comment on lines
+115
to
+118
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: Skipping validation on required fields allows incomplete identities. The current logic skips
The validation should run after setting required fields to ensure the final state is valid. 🐛 Proposed fix function setAttribute(
uint256 tokenId,
string calldata key,
bytes calldata value
) external onlyTokenOwner(tokenId) notCompromised(tokenId) {
- bytes32 keyHash = keccak256(abi.encodePacked(key));
-
_setAttribute(tokenId, key, value);
-
- // skip validation if the updated attribute is one of the required fields, since
- if (keyHash != NAME_KEY && keyHash != EMAIL_KEY && keyHash != PHONE_KEY) {
- _validateRequiredFields(tokenId);
- }
+ _validateRequiredFields(tokenId);
}🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -108,9 +139,13 @@ contract IdentityToken is ERC721, IIdentityToken { | |
| ) external onlyTokenOwner(tokenId) notCompromised(tokenId) { | ||
| if (keys.length != values.length) revert Errors.ArrayLengthMismatch(); | ||
|
|
||
| uint8 shouldValidate = 0; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove unused variable.
🧹 Proposed fix ) external onlyTokenOwner(tokenId) notCompromised(tokenId) {
if (keys.length != values.length) revert Errors.ArrayLengthMismatch();
- uint8 shouldValidate = 0;
-
for (uint256 i = 0; i < keys.length; i++) {
_setAttribute(tokenId, keys[i], values[i]);
}
_validateRequiredFields(tokenId);
}🤖 Prompt for AI Agents |
||
|
|
||
| for (uint256 i = 0; i < keys.length; i++) { | ||
| _setAttribute(tokenId, keys[i], values[i]); | ||
| } | ||
|
|
||
| _validateRequiredFields(tokenId); | ||
| } | ||
|
Comment on lines
89
to
149
|
||
|
|
||
| /** | ||
|
|
@@ -120,6 +155,27 @@ contract IdentityToken is ERC721, IIdentityToken { | |
| _setAttribute(tokenId, "name", bytes(name)); | ||
| } | ||
|
|
||
| /** | ||
| * @dev Convenience setter for the "email" / "phone" attribute. | ||
| */ | ||
| function setContact( | ||
| uint256 tokenId, | ||
| string calldata email, | ||
| string calldata phone | ||
| ) external onlyTokenOwner(tokenId) notCompromised(tokenId) { | ||
| if (bytes(email).length == 0 && bytes(phone).length == 0) { | ||
| revert Errors.MissingContact(); | ||
| } | ||
|
|
||
| if (bytes(email).length != 0) { | ||
| _setAttribute(tokenId, "email", bytes(email)); | ||
| } | ||
|
|
||
| if (bytes(phone).length != 0) { | ||
| _setAttribute(tokenId, "phone", bytes(phone)); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @dev Convenience setter for the "github" attribute. | ||
| */ | ||
|
|
@@ -128,6 +184,18 @@ contract IdentityToken is ERC721, IIdentityToken { | |
| string calldata github | ||
| ) external onlyTokenOwner(tokenId) notCompromised(tokenId) { | ||
| _setAttribute(tokenId, "github", bytes(github)); | ||
| _validateRequiredFields(tokenId); | ||
| } | ||
|
|
||
| function deleteAttribute( | ||
| uint256 tokenId, | ||
| string calldata key | ||
| ) external onlyTokenOwner(tokenId) notCompromised(tokenId) { | ||
| bytes32 keyHash = keccak256(abi.encodePacked(key)); | ||
|
|
||
| delete attributes[tokenId][keyHash]; | ||
|
|
||
| emit Events.AttributeDeleted(tokenId, keyHash); | ||
| } | ||
|
Comment on lines
+190
to
199
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: Deleting 🐛 Proposed fix function deleteAttribute(
uint256 tokenId,
string calldata key
) external onlyTokenOwner(tokenId) notCompromised(tokenId) {
bytes32 keyHash = keccak256(abi.encodePacked(key));
delete attributes[tokenId][keyHash];
emit Events.AttributeDeleted(tokenId, keyHash);
+
+ _validateRequiredFields(tokenId);
}As per coding guidelines, "Ensure failure paths and revert scenarios are explicitly handled and validated." 🤖 Prompt for AI Agents |
||
|
|
||
| /** | ||
|
|
@@ -184,4 +252,56 @@ contract IdentityToken is ERC721, IIdentityToken { | |
|
|
||
| emit Events.AttributeSet(tokenId, keyHash, key, value); | ||
| } | ||
|
|
||
| // Identity helpers | ||
|
|
||
| /// @notice Returns true if the address owns any identity token. | ||
| function hasIdentity(address owner) external view returns (bool) { | ||
| return balanceOf(owner) > 0; | ||
| } | ||
|
|
||
| /// @notice Returns full metadata for a given token. | ||
| function getIdentity(uint256 tokenId) external view returns (DataTypes.Identity memory) { | ||
| address owner = _requireOwned(tokenId); | ||
| DataTypes.IdentityState storage state = identityStates[tokenId]; | ||
| return | ||
| DataTypes.Identity({ | ||
| tokenId: tokenId, | ||
| owner: owner, | ||
| isCompromised: state.isCompromised, | ||
| backupWallet: state.backupWallet, | ||
| pendingBackupWallet: state.pendingBackupWallet, | ||
| backupUnlockTime: state.backupUnlockTime, | ||
| validUntil: state.validUntil, | ||
| endorsementCount: endorsements[tokenId].length | ||
| }); | ||
| } | ||
|
|
||
| /// @notice Returns all token IDs owned by an address (0 or 1 given soulbound constraint). | ||
| function getIdentityByOwner(address owner) external view returns (uint256[] memory) { | ||
| uint256 tokenId = ownerToTokenId[owner]; | ||
| if (tokenId == 0) { | ||
| return new uint256[](0); | ||
| } | ||
| uint256[] memory result = new uint256[](1); | ||
| result[0] = tokenId; | ||
| return result; | ||
| } | ||
|
|
||
| /// @notice Returns true if the token has at least one active (non-revoked, non-expired) endorsement. | ||
| function isVerified(uint256 tokenId) external view returns (bool) { | ||
| DataTypes.Endorsement[] storage list = endorsements[tokenId]; | ||
| 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); | ||
| if (active) return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /// @notice Returns true if the token's validUntil is set and has passed. | ||
| function isExpired(uint256 tokenId) external view returns (bool) { | ||
| uint256 validUntil = identityStates[tokenId].validUntil; | ||
| return validUntil != 0 && block.timestamp > validUntil; | ||
|
Comment on lines
+302
to
+305
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This contract never writes 🤖 Prompt for AI Agents |
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Gas optimization: Pre-compute constant key hashes.
The function computes
keccak256(abi.encodePacked("name")),keccak256(abi.encodePacked("email")), andkeccak256(abi.encodePacked("phone"))on every call. Since these are static strings, declare them asconstant bytes32at the contract level (or reuse fromSchema.solif already defined) to save gas.♻️ Proposed refactor
Add constants at contract level:
Then refactor the function:
function _validateRequiredFields(uint256 tokenId) internal view { - bytes memory name = attributes[tokenId][keccak256(abi.encodePacked("name"))]; - bytes memory email = attributes[tokenId][keccak256(abi.encodePacked("email"))]; - bytes memory phone = attributes[tokenId][keccak256(abi.encodePacked("phone"))]; + bytes memory name = attributes[tokenId][KEY_NAME]; + bytes memory email = attributes[tokenId][KEY_EMAIL]; + bytes memory phone = attributes[tokenId][KEY_PHONE]; // Name is mandatory if (name.length == 0) revert Errors.MissingName(); // At least one contact method required if (email.length == 0 && phone.length == 0) { revert Errors.MissingContact(); } }🤖 Prompt for AI Agents