Security audit and production readiness fixes for Tansu smart contrac… #239
Conversation
✅ Deploy Preview for staging-tansu ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for tansu ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @Septimus4 thanks for doing that! Can you rebase on #240 or have a look at it? I added a bunch of things. |
|
@tupui, rebased and I added a commit to improve the pause logic: |
|
Thanks 🙏 I am at Meridian now, I will do my best to review quickly! |
tupui
added
rust
There was a problem hiding this comment.
Thank you for doing this. I have some minor things to address but otherwise that's great.
The intent is stricter governance (requirement that a majority of all eligible voters must approve), then code was correct and docs needed updating. If the intent is looser governance (only count explicit yes/no votes), then docs were correct and code needed changing. @tupui, I went with the code a source of truth and updated the doc, I can do the other way around.
Correct, the code is the source of truth and we want this supermajority 👍 Abstaining is a vote in itself and can be an outcome. We have accept/reject/abstain as possible outcomes.
- Maintainer authorization on
execute
Added explicit checks so only designated maintainers can trigger contract upgrades.
Why: Prevents unauthorized execution that could bypass governance decisions.
This is not needed because we this way the DAO can be decentralised in its operation. This is also why I added a collateral system so that anyone can open a proposal. Such mode of operation is classic in other DAO. Anyone can trigger the execution after the tally phase is done. The caveat in our case is with anonymous voting. Only a maintainer can have the private key, hence only that maintainer will be able to execute the proposal. I think this is ok and we can add some wording in the UI for that.
- Voting deadline enforcement
Enforced timestamp comparison againstenv.ledger().timestamp().
Why: Eliminates ambiguity about late votes and ensures proposals close predictably.
Good catch! That was a real bug there.
- Anonymous vote and seed validation
Enforced strict equality between votes and seeds arrays, with explicit errors.
Why: Protects against index errors, malformed submissions, and tally corruption.
Good input validation 👍
- BLS commitment verification
Added tests with known vectors and documented cryptographic assumptions (curve, RNG, serialization).
Why: Ensures anonymity mechanisms are verifiable and built on explicit, auditable assumptions.
Not sure if really needed. We can assume the SDK is doing the right thing and only need to test our handling of it.
- Pagination and gas limits
Introduced hard caps on page sizes and added gas-scaling tests.
Why: Protects against resource exhaustion and DoS vectors.
Good catch, I had missed one place indeed 👍
contracts/tansu/src/contract_dao.rs
Outdated
| if voter_max_weight == 0 { | ||
| panic_with_error!(&env, &errors::ContractErrors::UnknownMember); | ||
| } |
There was a problem hiding this comment.
I was not sure about that. But I guess if you do not register, voting anyway does nothing because your vote basically count for nothing at all and yes we get some DoS then.
contracts/tansu/src/contract_dao.rs
Outdated
| maintainer.require_auth(); | ||
| // Verify the maintainer is authorized for this project | ||
| crate::auth_maintainers(&env, &maintainer, &project_key); |
contracts/tansu/src/contract_dao.rs
Outdated
| }; | ||
|
|
||
| // Validate tallies and seeds have expected length (3: approve, reject, abstain) | ||
| if tallies_.len() != 3 || seeds_.len() != 3 { |
There was a problem hiding this comment.
ok but now we check these too many times. Here then in the proof etc. We just need to check once.
| if !(tallies.is_some() && seeds.is_some()) { | ||
| panic_with_error!(&env, &errors::ContractErrors::TallySeedError); | ||
| } | ||
| let tallies_ = tallies.unwrap(); |
There was a problem hiding this comment.
Some unwrap_or_else or else would be more succinct no?
contracts/tansu/src/contract_dao.rs
Outdated
| voted_abstain: u128, | ||
| _voted_abstain: u128, |
There was a problem hiding this comment.
Why the change? I would not do that.
| // Proposing an upgrade mutates state; require not paused. | ||
| Self::require_not_paused(env.clone()); |
There was a problem hiding this comment.
Even more important here. Admins would even pause the contract to do a migration to prevent anything bad from happening.
| // Approving an upgrade mutates state; require not paused. | ||
| Self::require_not_paused(env.clone()); |
| // Executing an upgrade should respect pause state. | ||
| Self::require_not_paused(env.clone()); |
| To cast a vote on a proposal, users must: | ||
|
|
||
| - Be registered as a member using `add_member` | ||
| - Have one or more badges assigned for the project (see [Membership & Badges](./membership.mdx)) |
There was a problem hiding this comment.
People can have no badges, they just get weight as 1. Maybe I should revisit the values. I will think about that for some follow ups.
There was a problem hiding this comment.
These belong to the existing test files I think.
|
FYI I am also going to push a small update to add a collateral on the votes themselves. All that should really prevent DoS issues. |
|
I have merged my changes on main for the collateral on votes. |
Okay I will rebase and check |
There was a problem hiding this comment.
Pull Request Overview
This PR strengthens the Tansu smart contract governance system by addressing security vulnerabilities, improving error handling, and clarifying governance rules. It focuses on production readiness and auditability enhancements.
Key Changes:
- Replaced unsafe
unwrap()calls with proper error handling - Added strict validation for voting deadlines, thresholds, and anonymous voting parameters
- Clarified supermajority governance model where abstentions count toward the denominator
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| website/docs/developers/governance.mdx | Documents supermajority governance rules and voting requirements |
| contracts/tansu/src/tests/test_security.rs | Adds comprehensive security tests for edge cases and malformed inputs |
| contracts/tansu/src/tests/mod.rs | Includes new security test module |
| contracts/tansu/src/contract_tansu.rs | Improves constructor safety, adds threshold validation, and strengthens upgrade controls |
| contracts/tansu/src/contract_membership.rs | Simplifies nested pattern matching for better readability |
| contracts/tansu/src/contract_dao.rs | Enhances vote validation, deadline enforcement, and removes unsafe operations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
github-project-automation
bot
moved this from In progress
to Done
in SCF Journey - Tansu
|
With all the rebase I had issues, I will open it updated and cleaned soon |
|
Ok no worries 👍 thanks for following up. My objective is to attempt mainnet this week. I sort of prepared the rest on Netlify, Cloudflare and the registrar. Only need to deploy the contract basically. |
Strengthens the governance contract by addressing unsafe patterns, clarifying rules, and adding protective checks. It improves security, maintainability, and auditability.
I focused only on code inside contracts/tansu/src
For Point 7 specifically, my understanding:
Total votes used as denominator included abstentions. That means abstaining acted like a silent “no.”
Example: with 10 voters, 4 yes, 0 no, 6 abstain → 40% yes → proposal fails.
Doc description (before my update): majority was described as ratio of yes over (yes + no), meaning abstentions are ignored. Same case → 4 yes, 0 no → 100% yes → proposal passes.
The intent is stricter governance (requirement that a majority of all eligible voters must approve), then code was correct and docs needed updating. If the intent is looser governance (only count explicit yes/no votes), then docs were correct and code needed changing. @tupui, I went with the code a source of truth and updated the doc, I can do the other way around.
Maintainer authorization on
executeAdded explicit checks so only designated maintainers can trigger contract upgrades.
Why: Prevents unauthorized execution that could bypass governance decisions.
Voting deadline enforcement
Enforced timestamp comparison against
env.ledger().timestamp().Why: Eliminates ambiguity about late votes and ensures proposals close predictably.
Anonymous vote and seed validation
Enforced strict equality between votes and seeds arrays, with explicit errors.
Why: Protects against index errors, malformed submissions, and tally corruption.
Removal of unsafe
unwrap()Replaced all reachable
unwrap()calls with guarded branches and error returns.Why: Prevents contract panics that could be exploited for denial of service.
Logical operator corrections
Replaced bitwise
|and&with logical||and&&.Why: Ensures correct boolean evaluation and short-circuiting, reducing logic errors.
Governance threshold validation
Rejected thresholds of 0 or greater than the number of maintainers.
Why: Prevents governance lockups or trivial approvals that undermine the system.
Majority formula clarification
Documented and aligned behavior on how abstentions affect vote outcomes.
Why: Removes ambiguity between documentation and implementation, ensuring predictable governance.
Timestamp boundary tests
Added explicit test for voting at exactly the deadline tick.
Why: Clarifies off-by-one edge cases and prevents disputes around deadline interpretation.
Pattern simplification in
vote()Rewrote nested
if letconstructs into clear conditionals.Why: Improves readability, reduces audit complexity, and ensures stability across Rust versions.
BLS commitment verification
Added tests with known vectors and documented cryptographic assumptions (curve, RNG, serialization).
Why: Ensures anonymity mechanisms are verifiable and built on explicit, auditable assumptions.
Pagination and gas limits
Introduced hard caps on page sizes and added gas-scaling tests.
Why: Protects against resource exhaustion and DoS vectors.