Drop the strict ascending-address ordering requirement on multi-sig upgrade approvals

[N-thnI]

Description

Remove an undocumented ordering constraint that can deadlock a pending upgrade.

Problem Statement

approve_upgrade rejects any approval whose address does not sort strictly after the last recorded one (src/contracts/upgrade.rs:236-240, and identically at 153-157):

if let Some(previous) = approvals.last() {
    if previous >= signer {
        return Err(ContractError::InvalidUpgradeConfig);
    }
}

Duplicate approvals are already rejected two lines above by approvals.contains(signer.clone()), so this adds nothing except a hidden constraint on the order signers happen to transact in.

Verified against current main: with signers [A, B, C] sorted ascending and threshold 2, if B proposes first, A's subsequent approve_upgrade returns Err(InvalidUpgradeConfig) — and there is no way to withdraw B's approval, so the proposal can only ever be completed by C. With threshold equal to the signer count, a single out-of-order proposal deadlocks the upgrade until an admin calls cancel_upgrade.

It's also undocumented: the # Errors list at src/contracts/upgrade.rs:203-206 names only NotUpgradeSigner, NoPendingUpgrade, and AlreadyApproved. Every test in tests/upgrade.rs approves in ascending order, so the case is untested.

Proposed Changes

• Delete the previous >= signer check from both approve_upgrade and propose_upgrade; rely on approvals.contains(...) for dedup
• Keep is_strictly_sorted_addresses on the set_upgrade_signers configuration list (src/contracts/upgrade.rs:63) — canonical ordering is meaningful there
• If the sorted-approvals invariant is genuinely wanted for cheap membership checks, insert in sorted position rather than rejecting

Technical Implementation Scaffolding

• Target Repository: vero-core-contracts
• Target Path: src/contracts/upgrade.rs
• Branch Naming: fix/issue--upgrade-approval-ordering
• Authority Context: Security-sensitive — contract upgrade governance

Acceptance Criteria

• With sorted signers [A, B, C] and threshold 2, propose_upgrade(B) followed by approve_upgrade(A) succeeds
• Approving twice from the same address still returns AlreadyApproved
• All three signers can approve in any of the 6 permutations and execute_upgrade succeeds in each

Definition of Done

• Reviewed by lead maintainer
• Pull request merged via verified status check

[MAN7A-afk]

Hello maintainer, Please Assign me to this task

[safal207]

Hi @N-thnI — I’d like to take #304.

The existing approvals.contains(...) check already enforces signer uniqueness, so rejecting an address solely because it sorts before the previous approver adds no security property and can instead deadlock a valid upgrade.

My implementation would:

• Remove the order-dependent rejection from both propose_upgrade and approve_upgrade, while preserving canonical ordering validation in set_upgrade_signers.
• Add the exact B → A threshold-two regression case and table-driven coverage for all six approval permutations of [A, B, C].
• Confirm duplicate approval still returns AlreadyApproved, including the full-signers-threshold case where an out-of-order proposal currently becomes stuck.
• Run the full Rust/Soroban test and lint suite with no unrelated governance changes.

I have recent merged Soroban work involving authorization rules, state-transition invariants, and adversarial failure-path tests.

I can open a focused draft PR within 24 hours after assignment. I’ll wait for formal maintainer confirmation before starting.

[shobhamerabacha-star]

Hello! I'd like to work on this issue.

I would love to take this on. My planned approach:

• Implement Drop the strict ascending-address ordering requirement on multi-sig upgrade approvals with explicit require_auth checks, clean storage layouts, and robust Soroban contract logic.
• Add comprehensive unit tests verifying both happy-path invocations and unauthorized revert scenarios.

I am ready to start immediately—please assign this to me!