Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe update modifies several NEAR contracts by introducing explicit ownership management and updating documentation with revised build and test instructions. Both the guard and JWT contracts now include an owner field, initialization, and methods to enforce ownership checks, such as verifying the caller and allowing ownership transfers. In addition, a new multisig package ("multisig2") has been added with complete configuration files, build/test scripts, comprehensive documentation, and new data structures and functions for managing multisignature requests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Contract
User->>Contract: Call sensitive method (e.g., change_owner)
Contract->>Contract: Execute only_owner check
alt Caller is owner
Contract->>Contract: Proceed with the operation
Contract-->>User: Return success response
else Caller is not owner
Contract-->>User: Return error/panic
end
sequenceDiagram
participant Member
participant MultisigContract
participant OtherMember
Member->>MultisigContract: Submit multisig request (add_request)
MultisigContract-->>Member: Return request ID
OtherMember->>MultisigContract: Confirm the request (confirm)
MultisigContract->>MultisigContract: Check if confirmations meet threshold
alt Threshold met
MultisigContract-->>Member: Execute the request
else
MultisigContract-->>OtherMember: Await additional confirmations
end
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (15)
contracts/fa-guard/tests/test_integration.rs (1)
72-108: Ownership tests ensure robust coverage.
Thetest_ownerfunction correctly verifies initial ownership, ownership transfer, and updated ownership checks. Consider adding a negative scenario to confirm that non-owners cannot change the owner.contracts/fa-guard/src/lib.rs (3)
46-54: Consider validating new owner
Thechange_ownerimplementation is correct. Optionally, you might ensure that thenew_owneris not the zero address or the same as the current owner, to avoid mistakes.
125-129: Optional enhancement of callback error handling
While the implementation logs errors and returns false on failure, you might consider more detailed error handling or custom logic.
165-167: Basic owner test
The test confirms thatowner()returns the current account ID. Optionally, consider adding a test to validate the newchange_ownermethod.contracts/multisig2/Cargo.toml (1)
1-29: LGTM! Package configuration is properly set up for a NEAR smart contract.The Cargo.toml file is correctly configured with appropriate dependencies and optimized settings for smart contract deployment and testing.
However, I have two minor suggestions:
- The package is using near-sdk version "4.0.0-pre.4", which is a pre-release version. Consider updating to a stable version if available to avoid potential breaking changes.
- Consider upgrading from Rust edition 2018 to 2021 to access newer Rust features.
contracts/multisig2/build.sh (1)
1-6: LGTM! Build script is well-structured.The script correctly sets up the build environment with appropriate flags for WebAssembly compilation and optimization.
Consider adding a directory check to ensure the destination exists:
#!/bin/bash set -e RUSTFLAGS='-C link-arg=-s' cargo +stable build --target wasm32-unknown-unknown --release +mkdir -p ./res/ cp target/wasm32-unknown-unknown/release/multisig2.wasm ./res/contracts/multisig2/tests/general.rs (1)
91-91: Remove commented-out debug assertionThere's a commented-out assertion that appears to be debug code and should be removed.
- // assert!(1 == 2, "aloha ");contracts/multisig2/README.md (7)
6-8: Address Markdown indentation style issues.Several lines in the README use indentation that violates the preferred style (per the markdownlint MD007 rule). This can reduce readability and may confuse some Markdown parsers.
You can fix these by removing the extra leading space before the list markers, for example:
- - Set K out of N multi sig scheme - - Request to sign transfers, function calls, adding and removing keys. - - Any of the access keys... +- Set K out of N multi sig scheme +- Request to sign transfers, function calls, adding and removing keys. +- Any of the access keys...And similarly adjust the list items at lines 124-130 to have a consistent level of indentation (zero or one space aligned with your chosen style).
Also applies to: 124-130
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
6-6: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
7-7: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
8-8: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
12-13: Use articles and proper phrasing for clarity.The static analysis suggests adding missing articles ("a"/"the") and replacing “being setup” with “being set up”:
-Multisig uses set of `FunctionCall` `AccessKey`s and account ids as a set of allowed N members. -When contract is being setup, it should be initialized with set of members... +Multisig uses a set of `FunctionCall` `AccessKey`s and account IDs as a set of allowed N members. +When the contract is being set up, it should be initialized with a set of members...🧰 Tools
🪛 LanguageTool
[uncategorized] ~12-~12: Possible missing article found.
Context: ...g implementation details Multisig uses set ofFunctionCallAccessKeys and acco...(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~13-~13: Possible missing article found.
Context: ...ds as a set of allowed N members. When contract is being setup, it should be initialize...(AI_HYDRA_LEO_MISSING_A)
[grammar] ~13-~13: Make sure that the noun ‘setup’ is correct. Did you mean the past participle “set up”?
Context: ...owed N members. When contract is being setup, it should be initialized with set of m...(BE_VB_OR_NN)
[uncategorized] ~13-~13: Possible missing article found.
Context: ...ng setup, it should be initialized with set of members that will be initially manag...(AI_HYDRA_LEO_MISSING_THE)
20-20: Add missing article in the phrase "There are a number of…".-There are number of different request types that multisig can confirm... +There are a number of different request types that multisig can confirm...🧰 Tools
🪛 LanguageTool
[grammar] ~20-~20: It seems that an article is missing. Did you mean “a number of”?
Context: ... Initialization ### Request There are number of different request types that multisig c...(BUNCH_OF)
127-127: Fix verb form reference.The phrase "request is already has been confirmed and already is executing" is grammatically incorrect. Consider this revision:
-'confirm' fails if request is already has been confirmed and already is executing... +'confirm' fails if the request has already been confirmed and is currently executing...🧰 Tools
🪛 LanguageTool
[grammar] ~127-~127: The verb form seems incorrect.
Context: ... of request.confirmfails if request is already has been confirmed and already is executing...(IS_VBZ)
[uncategorized] ~127-~127: Possible missing comma found.
Context: ...ready has been confirmed and already is executing which is determined ifconfirmations...(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
127-127: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
129-129: Insert a comma for clarity in the callback explanation.-If no - stops executing the request and return failure. +If no, it stops executing the request and returns failure.🧰 Tools
🪛 LanguageTool
[uncategorized] ~129-~129: Possible missing comma found.
Context: ...ks if promise executed successfully: if no - stops executing the request and retur...(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
129-129: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
225-225: Specify a language for fenced code blocks.Markdown best practices (MD040) recommend explicitly declaring the language in fenced code blocks. For example:
-``` +```bash near view multisig.illia get_num_confirmationsAnd similarly for line 254. This helps with syntax highlighting and readability. Also applies to: 254-254 <details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 225-225: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> --- `252-252`: **Use a heading style instead of emphasis for "Create an account".** ```diff -__Create an account__ +### Create an accountHeadings are more accessible and consistent with the general Markdown style guidelines.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
252-252: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
contracts/multisig2/src/lib.rs (1)
224-290: Consider logging actions or using “env::log_str” macros for auditing.Within
execute_request, you’re performing multiple critical actions (transfers, contract deployments, etc.), but there’s currently no direct logging of successes or failures. Adding near-sdk logs or events can help with auditing and troubleshooting on-chain.Would you like to add logging to track each action’s success/failure?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
contracts/fa-guard-jwt/Cargo.lockis excluded by!**/*.lockcontracts/multisig2/Cargo.lockis excluded by!**/*.lockcontracts/multisig2/res/multisig2.wasmis excluded by!**/*.wasm
📒 Files selected for processing (12)
contracts/fa-guard-jwt-rs256/README.md(1 hunks)contracts/fa-guard-jwt/README.md(1 hunks)contracts/fa-guard-jwt/src/lib.rs(6 hunks)contracts/fa-guard-jwt/tests/test_integration.rs(3 hunks)contracts/fa-guard/src/lib.rs(4 hunks)contracts/fa-guard/tests/test_integration.rs(3 hunks)contracts/multisig2/Cargo.toml(1 hunks)contracts/multisig2/README.md(1 hunks)contracts/multisig2/build.sh(1 hunks)contracts/multisig2/src/lib.rs(1 hunks)contracts/multisig2/test.sh(1 hunks)contracts/multisig2/tests/general.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
contracts/fa-guard/tests/test_integration.rs (2)
contracts/fa-guard/src/lib.rs (2)
owner(42-44)owner(165-168)contracts/fa-guard-jwt/tests/test_integration.rs (1)
test_owner(82-122)
contracts/fa-guard-jwt/tests/test_integration.rs (2)
contracts/fa-guard-jwt/src/lib.rs (2)
owner(62-64)owner(273-276)contracts/fa-guard/tests/test_integration.rs (1)
test_owner(73-114)
contracts/fa-guard-jwt/src/lib.rs (1)
contracts/multisig2/src/lib.rs (2)
new(148-167)assert(136-140)
contracts/multisig2/tests/general.rs (1)
contracts/multisig2/src/lib.rs (2)
new(148-167)members(513-530)
🪛 LanguageTool
contracts/multisig2/README.md
[uncategorized] ~12-~12: Possible missing article found.
Context: ...g implementation details Multisig uses set of FunctionCall AccessKeys and acco...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~13-~13: Possible missing article found.
Context: ...ds as a set of allowed N members. When contract is being setup, it should be initialize...
(AI_HYDRA_LEO_MISSING_A)
[grammar] ~13-~13: Make sure that the noun ‘setup’ is correct. Did you mean the past participle “set up”?
Context: ...owed N members. When contract is being setup, it should be initialized with set of m...
(BE_VB_OR_NN)
[uncategorized] ~13-~13: Possible missing article found.
Context: ...ng setup, it should be initialized with set of members that will be initially manag...
(AI_HYDRA_LEO_MISSING_THE)
[grammar] ~20-~20: It seems that an article is missing. Did you mean “a number of”?
Context: ... Initialization ### Request There are number of different request types that multisig c...
(BUNCH_OF)
[grammar] ~127-~127: The verb form seems incorrect.
Context: ... of request. confirm fails if request is already has been confirmed and already is executing...
(IS_VBZ)
[uncategorized] ~127-~127: Possible missing comma found.
Context: ...ready has been confirmed and already is executing which is determined if confirmations ...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~129-~129: Possible missing comma found.
Context: ...ks if promise executed successfully: if no - stops executing the request and retur...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~263-~263: Did you mean “as”?
Context: ...ding for production. Remove the rlib like shown here: ```diff [lib] # Below is u...
(LIKE_SHOWN_TO_AS_SHOWN)
🪛 markdownlint-cli2 (0.17.2)
contracts/multisig2/README.md
6-6: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
7-7: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
8-8: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
124-124: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
125-125: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
126-126: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
127-127: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
128-128: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
129-129: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
130-130: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
135-135: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
139-139: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
225-225: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
252-252: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
254-254: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (50)
contracts/fa-guard-jwt-rs256/README.md (4)
3-3: Updated Project Description ClarityThe new description clearly states that this contract verifies JWT tokens signed with the RS256 algorithm. This aligns well with its intended security functionality.
10-11: Revised Build CommandThe build instruction has been updated to use
pnpm build, which modernizes the process and reflects the current tooling setup.
16-16: Alternative Build Option ProvidedIncluding the alternative command
cargo near build non-reproducible-wasmgives developers a useful fallback when a non-reproducible build is required.
22-23: Updated Testing CommandThe testing instructions now use
pnpm test, which is consistent with the updated build process and simplifies the developer experience.contracts/fa-guard-jwt/README.md (5)
1-1: Updated Project TitleThe header now reads
# fa-guard-jwt, which properly reflects the new project name and aligns with the governance and JWT verification capabilities introduced.
3-3: Enhanced Project DescriptionThe description now states that the contract acts as a guard proxy for JWT tokens verification. This change makes the purpose of the contract clear and accessible to developers.
10-11: Modernized Build CommandSwitching the build command to
pnpm buildmodernizes the installation process and ensures consistency with the overall project configuration.
16-16: Alternative Build OptionThe option
cargo near build non-reproducible-wasmis a welcome addition, providing an alternative for environments that require non-reproducible builds.
22-23: Refined Testing InstructionsUsing
pnpm testin the testing section enhances consistency with the build instructions and streamlines the developer experience.contracts/fa-guard/tests/test_integration.rs (4)
8-8: Owner account creation looks good.
This ensures that further contract initialization and actions reflect a proper ownership context.
11-18: Contract initialization with owner is correctly implemented.
Passing theowneraccount to the contract'sinitmethod ensures proper ownership setup.
21-21: Restricting guard addition to the owner is appropriate.
Invokingadd_guardvia theowneraccount maintains a clear access control pattern.
51-51: Restricting guard removal to the owner is consistent.
Callingremove_guardthrough theowneraligns with the intended access control.contracts/fa-guard-jwt/tests/test_integration.rs (7)
8-8: Owner account creation is correct.
This sets the foundation for restricting contract modifications to the designated account.
11-18: Initializing with owner is well-handled.
Callinginitwithownerensures the contract’s ownership logic is properly established from the start.
26-26: Registering implementations via owner is appropriate.
Making this method callable only by theownerhelps prevent unauthorized additions.
41-41: Unregistering implementations via owner is consistent.
Aligns with the same ownership guard used for registering.
51-62: Tests correctly validate non-owner register failure.
Verifying that unauthorized accounts cannot callregister_implementationis essential for security.
64-76: Tests correctly validate non-owner unregister failure.
Ensuring that unauthorized calls tounregister_implementationare denied further enhances the contract’s security.
81-120: Ownership tests demonstrate solid coverage.
Thetest_ownerfunction confirms the owner retrieval and change mechanism, reinforcing correct owner-based access control.contracts/fa-guard-jwt/src/lib.rs (12)
18-20: New fields for exponent and owner are clearly defined.
Persistingeandownerensures key management and ownership logic can be validated in all contract calls.
32-33: Default ownership set to current account is sensible.
This fallback ensures the contract is always initialized with a valid owner in the absence of explicit input.
42-50: Explicit initializer is well-structured.
Providing aninitmethod that setsownereliminates ambiguity and ensures clarity for contract deployment flows.
52-58: only_owner function effectively enforces ownership at runtime.
This helper is essential to guard restricted methods from unauthorized calls.
62-64: Owner getter method is minimal and correct.
Returning a clone ofowneris safe and meets typical best practices for retrieving state data.
66-74: Ownership transfer logic is clear.
change_ownerconfirms caller’s identity and updatesowner, preserving a secure ownership transition.
82-82: Restricting public key updates to owner is sound.
Ensures only the authorized party can modify critical signing/verification keys.
103-103: Owner-only registration ensures algorithm implementations remain vetted.
This prevents malicious or unauthorized algorithm entries.
112-112: Owner-only unregistration is aligned with access control.
Maintains consistent, secure restrictions on modifying algorithm implementations.
252-252: Expanded test struct for FaJwtGuard is coherent.
Includingownerin test data ensures thorough coverage and replicates real contract state.
267-267: Populatedimplementationswith an initial entry effectively tests non-empty scenarios.
Demonstrates correct initialization and reading of stored implementations.
273-275: Unit test forowneris straightforward.
Directly checks theowner()function to ensure it returns the expected account.contracts/fa-guard/src/lib.rs (13)
13-13: Addownerfield to contract structure
This addition cleanly introduces an explicit owner to support the new ownership logic.
21-21: Default owner set to contract’s own account
Usingenv::current_account_id()in the default implementation is a standard approach, making the contract its own owner if not otherwise initialized.
39-44: Public owner accessor
The accessor method is straightforward and clearly returns the stored owner account. No issues.
56-67:initmethod with explicit owner
This initialization flow is consistent with the ownership logic. The#[init]macro ensures single-call usage. Implementation looks good.
72-78: Clear documentation forget_guard
These doc comments accurately describe the function’s usage, returns, and panic conditions. No concerns.
83-88: Add guard doc comments
The documentation foradd_guardis thorough. It clearly notes that only the owner can invoke this method.
90-90: Ownership check inadd_guard
Enforcingonly_ownerhere ensures proper write protection over guard additions. Good approach.
95-99: Remove guard doc comments
The documentation is consistent withadd_guard, specifying the same ownership restriction and panic condition.
101-101: Ownership check inremove_guard
This ensures only the owner can remove entries, maintaining consistency withadd_guard.
108-113: Comprehensiveverifydoc comments
These comments clearly articulate the function’s purpose, arguments, and return type, aligning with the external call pattern.
153-153: Test with predefined guard
This test ensuresget_guardcorrectly returns an existing guard. The usage of theownerfield is consistent.
160-160: Test logic for guard absence
This test properly checks for a panic when requesting a missing guard. The ownership field setup is also correct.
169-169: No functional change
This trailing change appears to only close the tests module.contracts/multisig2/test.sh (1)
1-5: LGTM! Test script is straightforward and correct.The script properly builds the contract before running tests with the stable Rust toolchain.
contracts/multisig2/src/lib.rs (4)
295-315:confirmlogic looks solid; verify coverage with negative tests.The function properly checks for duplicate confirmations and finalizes the request if it meets the threshold. Ensure you have negative tests for:
- Reconfirming after threshold is met.
- Requests with insufficient confirmations never executing.
356-379: Validate removal of a member.When deleting a member, you remove their outstanding requests and their entry in
num_requests_pk. This is logical and helps maintain data consistency. No issues found here. Great job.
481-878: Comprehensive test coverage.Your tests include multiple scenarios (transfers, key additions/deletions, changing confirmations, etc.). This thorough approach significantly reduces the chance of regressions. Great work ensuring the correctness of core functionalities!
260-261:Details
❓ Verification inconclusive
Flagging full access key additions.
Line 260 warns about adding a full access key to the same contract, which could circumvent multisig controls. Consider ensuring that the contract cannot accidentally add a full access key to itself without an explicit override or special process.
Could you confirm that all callers are aware of this risk and that there is no hidden path that allows adding a full access key to the same account, effectively disabling the multisig functionality?
🏁 Script executed:
#!/bin/bash # Searching for invocation patterns where AddKey is used with no permission, verifying if there's a guarded path. rg -A 5 'AddKey'Length of output: 3024
Security Alert: Verify Full Access Key Addition Logic
The code at
contracts/multisig2/src/lib.rs, lines 260–261, callspromise.add_full_access_key(public_key.into())to add a full access key. This approach raises a security concern because it could inadvertently grant full permissions to the contract if misused. Please confirm the following:
- Awareness of the Risk: Ensure that every multisig request caller understands that specifying a full access key can potentially disable multisig controls.
- Guarded Addition: Verify that there is no unintended or hidden code path that could add a full access key to the contract itself without an explicit override or an additional safeguard. In particular, check that the logic around
MultiSigRequestAction::AddKey(and the correspondingassert_self_requestcall) consistently prevents unintentional self-targeting.The output from the grep shows that
AddKeyoperations are used in both test cases and documentation, suggesting deliberate usage. However, it remains essential to review the multisig implementation and test coverage to ensure that a full access key is added only when explicitly intended.
| /// Checks if the caller is the contract owner | ||
| /// # Panics | ||
| /// Panics if the caller is not the owner | ||
| fn only_owner(&self) { | ||
| assert!(env::signer_account_id() == self.owner, "Only the owner can call this function"); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm usage of signer_account_id vs. predecessor_account_id
While this ownership check is standard, ensure that using signer_account_id() (instead of predecessor_account_id()) aligns with your expected cross-contract call flow.
🌐 Web query:
What are the recommended best practices on NEAR for ownership checks using `env::signer_account_id()` vs. `env::predecessor_account_id()`?
💡 Result:
When implementing ownership checks in NEAR smart contracts, security best practices strongly recommend using env::predecessor_account_id() instead of env::signer_account_id() for access control. This approach mitigates risks in cross-contract call scenarios and follows NEAR's security patterns.
Key Recommendations:
Prefer env::predecessor_account_id() for Ownership Validation
- Immediate Caller Verification:
The predecessor represents the direct caller of your contract in the call chain[5][9]. When Alice calls Contract A, which then calls Contract B, Contract B sees Contract A as the predecessor, while Alice remains the signer[5]. - Prevents Middleman Attacks:
Checking only the signer allows malicious contracts to proxy calls while retaining the original signer's identity. Using predecessor ensures the immediate caller has explicit permissions[3][5].
fn require_owner_only(&self) {
require!(
env::predecessor_account_id() == self.owner_id,
"Owner-only method"
);
}Example from [3][4]
Limited Use Cases for env::signer_account_id()
- Original Transaction Initiator Tracking:
Useful for auditing the root source of transaction chains, not for access control[5][9]. - Non-State-Changing Operations:
May be appropriate for view functions or statistical tracking where security isn't critical[5].
Security Considerations
- Cross-Contract Call Risks:
A malicious contract could spoof the signer account in indirect calls if you rely onsigner_account_id()for permissions[5]. - Asset Protection:
When handling tokens/NFTs, always verify predecessor to prevent unauthorized transfers via proxy contracts[5][7]. - Consistency in Call Chains:
Predecessor checks ensure each contract in a call chain validates its direct caller's permissions[3][5].
Anti-Pattern Alert:
// UNSAFE - vulnerable to proxy attacks
require!(env::signer_account_id() == self.owner_id, "⚠️");NEAR's documentation and security advisories consistently emphasize this pattern, with predecessor_account_id() being used in 93% of access control implementations analyzed in official examples[4][5][9]. The approach aligns with blockchain security best practices for maintaining contract integrity in asynchronous, multi-contract environments.
Citations:
- 1: https://docs.near.org/chain-abstraction/chain-signatures
- 2: https://learnnear.club/near-live-contract-review-part-1-voting-contracts/
- 3: https://www.typedriven.ca/smart-contract-development-on-near-using-rust-part-2/
- 4: https://docs.near.org/build/smart-contracts/anatomy/best-practices
- 5: https://docs.near.org/tutorials/crosswords/beginner/actions
- 6: https://learnnear.club/smart-contract-development-on-near-using-rust-part-2/
- 7: https://webisoft.com/articles/near-protocol/
- 8: https://brson.github.io/2020/09/07/near-smart-contracts-rust
- 9: https://github.com/near/near-sdk-rs/blob/master/near-sdk/src/environment/env.rs
- 10: https://nordlayer.com/blog/blockchain-security-issues/
ACTION: Update Ownership Check to Use env::predecessor_account_id()
The current implementation uses env::signer_account_id() to verify ownership. However, NEAR's best practices strongly recommend using env::predecessor_account_id() in access control functions to ensure that the immediate caller is validated, mitigating potential proxy vulnerabilities in cross-contract interactions. If your design does not intentionally rely on the signer's identity (e.g., for auditing purposes), please update the ownership check accordingly.
- Current code:
assert!(env::signer_account_id() == self.owner, "Only the owner can call this function");
- Recommended change:
assert!(env::predecessor_account_id() == self.owner, "Only the owner can call this function");
If you intend to track the original caller for non-critical purposes, please document this design decision clearly.
| let new_args = json!( | ||
| { | ||
| "members": [ | ||
| { "public_key": multisig_signer.public_key() } | ||
| ], | ||
| "num_confirmations": 1} | ||
| ).to_string(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Limited testing of multisig functionality
The test initializes a multisig contract with only one member and requires only one confirmation, which doesn't fully test the multi-signature aspect of the contract.
Consider enhancing the test to include multiple members and require multiple confirmations to better test the multi-signature functionality:
let new_args = json!(
{
"members": [
- { "public_key": multisig_signer.public_key() }
+ { "public_key": multisig_signer.public_key() },
+ { "account_id": accounts(0) }
],
- "num_confirmations": 1}
+ "num_confirmations": 2}
).to_string();📝 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.
| let new_args = json!( | |
| { | |
| "members": [ | |
| { "public_key": multisig_signer.public_key() } | |
| ], | |
| "num_confirmations": 1} | |
| ).to_string(); | |
| let new_args = json!( | |
| { | |
| "members": [ | |
| { "public_key": multisig_signer.public_key() }, | |
| { "account_id": accounts(0) } | |
| ], | |
| "num_confirmations": 2} | |
| ).to_string(); |
| #[test] | ||
| fn setup_and_remove_multisig() { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Function name doesn't match test behavior
The test function is named setup_and_remove_multisig() but it doesn't actually test removing a multisig account.
Either update the function name to match its current behavior or implement functionality to test removing the multisig account.
#[test]
-fn setup_and_remove_multisig() {
+fn setup_and_test_multisig() {📝 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.
| #[test] | |
| fn setup_and_remove_multisig() { | |
| #[test] | |
| fn setup_and_test_multisig() { |
| @@ -0,0 +1,878 @@ | |||
| use std::collections::HashSet; | |||
There was a problem hiding this comment.
I would remove this contract from here since it does not necessary apply to the fast-auth domain itself.
The fast-auth defines a governance account that can be a multisig or any other type of externally owned account
2 participants
[TA-4415]: governance base setup
Changes 🛠️
contracts/fa-guard
ownercapabilities to contract.contracts/fa-guard-jwt
ownercapabilities to contract.contracts/multisig2
multisig2core contract fromnear/core-contracts.Summary by CodeRabbit