diff --git a/docs/CONTRACT_FUNCTIONALITY_REVIEW.md b/docs/CONTRACT_FUNCTIONALITY_REVIEW.md new file mode 100644 index 0000000..833266d --- /dev/null +++ b/docs/CONTRACT_FUNCTIONALITY_REVIEW.md @@ -0,0 +1,384 @@ +# Chamber Smart Contract Functionality Review + +**Review Date:** February 3, 2026 +**Contracts Reviewed:** Chamber.sol, Board.sol, Wallet.sol, Registry.sol +**Solidity Version:** 0.8.30 + +--- + +## Executive Summary + +The Chamber protocol is a sophisticated governance system that combines three core functionalities: +1. **ERC4626 Vault** - Tokenized asset deposits with share-based accounting +2. **Board Governance** - NFT-based delegation with dynamic director selection +3. **Multisig Wallet** - Quorum-based transaction approval and execution + +The contracts demonstrate solid architectural decisions and security practices, though there are areas for improvement in both functionality and security. + +--- + +## Contract Overview + +### 1. Chamber.sol (Main Contract) +The central contract combining vault, governance, and wallet functionality. + +### 2. Board.sol (Abstract) +Manages a sorted linked list for tracking delegations and determining board directors. + +### 3. Wallet.sol (Abstract) +Implements multisig transaction management with confirmation tracking. + +### 4. Registry.sol +Factory contract for deploying new Chamber instances via TransparentUpgradeableProxy. + +--- + +## Strengths + +### 1. Well-Designed Architecture + +**Modular Design** +- Clean separation of concerns with Board and Wallet as abstract base contracts +- Chamber composes all functionality through multiple inheritance +- Interfaces (`IChamber`, `IBoard`, `IWallet`) provide clear API boundaries + +**Upgradeability** +- Uses OpenZeppelin's battle-tested TransparentUpgradeableProxy pattern +- Each Chamber owns its own ProxyAdmin, enabling governance-controlled upgrades +- Storage gaps (`uint256[50] private __gap`) in all contracts prevent storage collisions +- Proper use of initializers with `_disableInitializers()` in constructors + +### 2. Robust Security Patterns + +**Reentrancy Protection** +- `ReentrancyGuardUpgradeable` on `executeTransaction` and `executeBatchTransactions` +- Custom `circuitBreaker` modifier in Board for linked list operations +- `preventReentry` modifier for delegation operations + +**CEI Pattern in Wallet** +```solidity +// Effect first, then interaction +transaction.executed = true; +(bool success, bytes memory returnData) = target.call{value: value}(data); +if (!success) { + transaction.executed = false; // Rollback on failure + revert IWallet.TransactionFailed(returnData); +} +``` + +**Comprehensive Input Validation** +- Zero address checks throughout +- Zero amount checks +- Token existence validation via `nft.ownerOf()` with try/catch +- Balance checks before operations + +**Director Verification** +- `isDirector` modifier verifies: + 1. TokenId is not zero + 2. Caller owns the NFT + 3. TokenId is in top seats (directors) + +### 3. ERC4626 Compliance with Extensions + +**Share-Based Delegation** +- Users deposit ERC20 tokens to receive shares +- Shares can be delegated to NFT tokenIds +- Delegation is locked on transfer (prevents transferring delegated tokens) + +**Override Protection** +```solidity +function transfer(address to, uint256 value) public override returns (bool) { + // Check delegation before transfer + if (ownerBalance - value < totalAgentDelegations[owner]) { + revert IChamber.ExceedsDelegatedAmount(); + } + // ... +} +``` + +### 4. Efficient Data Structures + +**Sorted Doubly Linked List** +- Maintains delegations sorted by amount (descending) +- Efficient O(n) insertion maintaining sort order +- O(1) access to top directors via head pointer +- Clean removal with proper link updates + +**Quorum Calculation** +```solidity +function _getQuorum() internal view returns (uint256) { + return 1 + (seats * 51) / 100; +} +``` +Simple 51% majority formula that's clear and gas-efficient. + +### 5. Governance Features + +**7-Day Timelock for Seat Changes** +- Proposals require quorum support +- 7-day waiting period before execution +- Prevents rushed governance changes + +**Batch Operations** +- `submitBatchTransactions`, `confirmBatchTransactions`, `executeBatchTransactions` +- Reduces gas costs for multiple operations +- Atomic failure handling within batches + +### 6. Event-Driven Design + +**Comprehensive Events** +- All state changes emit events +- Indexed parameters for efficient filtering +- Separate events in Chamber for better tracking + +### 7. Registry Pattern + +**Factory Deployment** +- Centralized deployment via Registry +- Tracks all deployed Chambers +- Pagination support (`getChambers(limit, skip)`) +- Verification function (`isChamber(address)`) + +--- + +## Weaknesses + +### 1. Gas Inefficiencies + +**O(n) Linked List Operations** +- Every delegation change requires traversal to find insertion position +- `getTop()` traverses from head each call +- `getDelegations()` iterates through entire list +- With 100 nodes maximum, this is manageable but not optimal + +**Recommendation:** Consider a heap-based structure or skip list for O(log n) operations if scaling beyond 100 nodes is needed. + +**Director Check in Modifier** +```solidity +while (current != 0 && remaining > 0) { + if (current == tokenId) { + _; + return; + } + current = nodes[current].next; + remaining--; +} +``` +This iterates up to `seats` times for every director action. + +### 2. Limited Transaction Lifecycle + +**No Transaction Cancellation** +- Transactions can only be confirmed or revoked +- No way to delete stale/unwanted transactions +- Transaction array grows unbounded + +**No Expiration Mechanism** +- Transactions remain pending indefinitely +- Directors can confirm old transactions at any time +- No way to "expire" outdated proposals + +**Recommendation:** Add transaction expiration (e.g., 30-day validity) and explicit cancellation by submitter. + +### 3. NFT Transfer Vulnerability + +**Directors Can Change Mid-Transaction** +- If an NFT is transferred, the new owner becomes director +- Previous confirmations remain valid +- Could lead to unexpected execution scenarios + +```solidity +// Scenario: +// 1. Director A confirms transaction +// 2. Director A transfers NFT to B +// 3. B is now director, A's confirmation counts toward quorum +// 4. B could be unaware of pending transaction +``` + +**Recommendation:** Consider invalidating confirmations when NFT ownership changes, or require re-confirmation. + +### 4. Delegation Attack Vectors + +**Flash Loan Governance Attack** +- User could flash loan tokens, deposit, delegate, and become director +- Then submit/confirm transactions in same block +- 7-day timelock only protects seat changes, not transaction execution + +**Recommendation:** Add a minimum delegation age requirement for director privileges. + +**Delegation Without Balance Lock** +- User delegates tokens to NFT tokenId +- User can still use shares in ERC4626 `redeem` (withdrawal) +- Only `transfer` and `transferFrom` are protected + +```solidity +// Current protection only in transfer: +if (fromBalance - value < totalAgentDelegations[from]) { + revert IChamber.ExceedsDelegatedAmount(); +} +// But ERC4626.redeem() and withdraw() are not protected +``` + +**Recommendation:** Override `withdraw()` and `redeem()` to check delegation constraints. + +### 5. Quorum Edge Cases + +**Quorum with Zero Seats** +- `_getQuorum()` returns 1 when seats = 0 +- However, initialization prevents seats = 0 +- Edge case handled but worth noting + +**Quorum Changes During Proposal** +- If seats increase, quorum increases +- Pending transactions may no longer have enough confirmations +- Could strand transactions + +### 6. Self-Upgrade Restrictions + +**Only upgradeImplementation Allowed** +```solidity +if (target == address(this)) { + if (selector != UPGRADE_SELECTOR) { + revert IChamber.InvalidTransaction(); + } +} +``` +- Chamber cannot call its own view functions via transaction +- Limits flexibility for certain governance patterns + +### 7. Limited Error Information + +**Generic Error Types** +- Many functions use the same error for different failure modes +- `NotDirector` is used for both "not owner" and "not in top seats" +- Makes debugging more difficult + +**Recommendation:** Add more specific error messages with parameters. + +### 8. Missing Features + +**No Delegate Incentives** +- Users delegate tokens but receive no reward +- No mechanism to share governance rewards with delegators + +**Single Asset Limitation** +- Chamber only holds one ERC20 as vault asset +- No mechanism to manage multiple tokens +- ETH transfers work, but no multi-token treasury + +**No Pause Mechanism** +- No way to pause operations in emergency +- Circuit breaker only prevents reentry, not pause + +**No Quorum Override** +- Fixed quorum formula +- No way to adjust without contract upgrade + +### 9. Registry Limitations + +**No Chamber Removal** +- Chambers cannot be removed from registry +- No deprecation mechanism +- Could accumulate abandoned chambers + +**No Implementation Versioning** +- Single `implementation` address +- No version tracking for upgrades +- Harder to audit deployment history + +### 10. Confirmation System Limitations + +**One Confirmation Per TokenId** +```solidity +mapping(uint256 => mapping(uint256 => bool)) internal isConfirmed; +``` +- Same NFT cannot confirm twice (correct) +- But confirmation is tied to tokenId, not owner +- If same owner has multiple NFTs in top seats, each can confirm separately + +**uint8 Confirmation Counter** +```solidity +uint8 confirmations; +``` +- Maximum 255 confirmations +- With MAX_SEATS = 20, this is fine +- But limits future expansion + +--- + +## Security Considerations + +### Positive Security Findings + +1. **Solidity 0.8.30**: Built-in overflow protection +2. **OpenZeppelin Dependencies**: Battle-tested libraries +3. **No Floating Pragma**: Pinned to exact version +4. **Proper Visibility**: Functions appropriately scoped +5. **Storage Gaps**: Future upgrade compatibility + +### Areas Requiring Attention + +1. **ERC4626 Donation Attack**: First depositor could inflate share price +2. **View Function Gas**: Some view functions could exceed block gas limit with many nodes +3. **Oracle-Free Design**: No external price feeds (reduces attack surface) + +--- + +## Recommendations Summary + +### High Priority + +1. **Override ERC4626 withdraw/redeem** to enforce delegation constraints +2. **Add transaction expiration** to prevent stale transaction execution +3. **Consider flash loan protection** for governance actions + +### Medium Priority + +4. **Add pause mechanism** for emergency situations +5. **Improve error granularity** with specific error types +6. **Add NFT transfer hooks** to handle director changes + +### Low Priority + +7. **Gas optimization** for linked list operations +8. **Multi-asset treasury support** +9. **Registry versioning and deprecation** + +--- + +## Conclusion + +The Chamber protocol demonstrates solid engineering with a well-thought-out architecture. The combination of ERC4626 vault, NFT-based governance, and multisig functionality creates a flexible system for decentralized treasury management. + +The main strengths are: +- Clean modular architecture +- Strong reentrancy protections +- Proper upgradeability patterns +- Comprehensive input validation + +The main weaknesses are: +- Missing delegation checks on ERC4626 withdraw/redeem +- No transaction expiration/cancellation +- Potential flash loan governance attacks +- Gas inefficiency in linked list operations + +Overall, the contracts are well-designed for their intended purpose, but would benefit from the recommended improvements before production deployment on mainnet. + +--- + +## Appendix: Contract Metrics + +| Contract | Lines of Code | External Dependencies | +|----------|---------------|----------------------| +| Chamber.sol | ~688 | ERC4626Upgradeable, ReentrancyGuardUpgradeable | +| Board.sol | ~389 | None (abstract) | +| Wallet.sol | ~202 | None (abstract) | +| Registry.sol | ~184 | AccessControl, Initializable, TransparentUpgradeableProxy | + +| Feature | Implementation | +|---------|---------------| +| Quorum Formula | 1 + (seats * 51) / 100 | +| Max Seats | 20 | +| Max Nodes | 100 | +| Seat Change Timelock | 7 days | +| Storage Gaps | 50 slots per contract | diff --git a/foundry.lock b/foundry.lock new file mode 100644 index 0000000..a4821e4 --- /dev/null +++ b/foundry.lock @@ -0,0 +1,14 @@ +{ + "lib/forge-std": { + "rev": "1eea5bae12ae557d589f9f0f0edae2faa47cb262" + }, + "lib/openzeppelin-contracts": { + "branch": { + "name": "release-v5.0", + "rev": "69c8def5f222ff96f2b5beff05dfba996368aa79" + } + }, + "lib/openzeppelin-contracts-upgradeable": { + "rev": "fa525310e45f91eb20a6d3baa2644be8e0adba31" + } +} \ No newline at end of file diff --git a/src/Chamber.sol b/src/Chamber.sol index c450463..54af8b7 100644 --- a/src/Chamber.sol +++ b/src/Chamber.sol @@ -7,6 +7,7 @@ import {IChamber} from "src/interfaces/IChamber.sol"; import {IWallet} from "src/interfaces/IWallet.sol"; import {IERC20} from "lib/openzeppelin-contracts/contracts/interfaces/IERC20.sol"; import {IERC721} from "lib/openzeppelin-contracts/contracts/interfaces/IERC721.sol"; +import {IERC4626} from "lib/openzeppelin-contracts/contracts/interfaces/IERC4626.sol"; import { ERC4626Upgradeable } from "lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/extensions/ERC4626Upgradeable.sol"; @@ -38,11 +39,17 @@ contract Chamber is ERC4626Upgradeable, ReentrancyGuardUpgradeable, Board, Walle /// @notice Mapping to track total delegated amount per agent mapping(address => uint256) private totalAgentDelegations; + /// @notice Mapping to track when each agent first delegated (for director age requirement) + mapping(address => uint256) private agentFirstDelegationTime; + /// @dev Events and errors are defined in IChamber interface /// Constants uint256 private constant MAX_SEATS = 20; + /// @notice Minimum time an agent must have delegated before becoming a director (1 day) + uint256 public constant MINIMUM_DELEGATION_AGE = 1 days; + /// @notice Function selector for upgradeImplementation(address,bytes) bytes4 private constant UPGRADE_SELECTOR = 0xc89311b6; @@ -76,7 +83,7 @@ contract Chamber is ERC4626Upgradeable, ReentrancyGuardUpgradeable, Board, Walle nft = IERC721(erc721Token); _setSeats(0, seats); - version = "0.4"; + version = "0.5"; } /** @@ -104,6 +111,11 @@ contract Chamber is ERC4626Upgradeable, ReentrancyGuardUpgradeable, Board, Walle agentDelegation[msg.sender][tokenId] += amount; totalAgentDelegations[msg.sender] += amount; + // Track first delegation time for director age requirement + if (agentFirstDelegationTime[msg.sender] == 0) { + agentFirstDelegationTime[msg.sender] = block.timestamp; + } + // Update board state _delegate(tokenId, amount); @@ -500,6 +512,7 @@ contract Chamber is ERC4626Upgradeable, ReentrancyGuardUpgradeable, Board, Walle /// @notice Modifier to restrict access to only directors /// @dev Checks if the caller owns a tokenId that is in the top seats + /// @dev Also enforces minimum delegation age requirement to prevent flash loan attacks /// @param tokenId The NFT token ID to check for directorship modifier isDirector(uint256 tokenId) { // Prevent zero tokenId @@ -508,6 +521,12 @@ contract Chamber is ERC4626Upgradeable, ReentrancyGuardUpgradeable, Board, Walle // Check if tokenId exists and is owned by caller if (nft.ownerOf(tokenId) != msg.sender) revert IChamber.NotDirector(); + // Check minimum delegation age requirement (prevents flash loan attacks) + uint256 firstDelegationTime = agentFirstDelegationTime[msg.sender]; + if (firstDelegationTime == 0 || block.timestamp < firstDelegationTime + MINIMUM_DELEGATION_AGE) { + revert IChamber.DelegationTooRecent(); + } + // Check if tokenId is in top seats uint256 current = head; uint256 remaining = _getSeats(); @@ -638,6 +657,67 @@ contract Chamber is ERC4626Upgradeable, ReentrancyGuardUpgradeable, Board, Walle return true; } + /// ERC4626 OVERRIDES /// + + /** + * @notice Withdraws assets from the vault + * @dev Overrides ERC4626 withdraw to check delegation constraints + * @param assets The amount of assets to withdraw + * @param receiver The address to receive the assets + * @param owner The address of the share owner + * @return shares The amount of shares burned + */ + function withdraw(uint256 assets, address receiver, address owner) + public + override(ERC4626Upgradeable, IERC4626) + returns (uint256) + { + uint256 shares = previewWithdraw(assets); + _checkDelegationConstraint(owner, shares); + return super.withdraw(assets, receiver, owner); + } + + /** + * @notice Redeems shares from the vault + * @dev Overrides ERC4626 redeem to check delegation constraints + * @param shares The amount of shares to redeem + * @param receiver The address to receive the assets + * @param owner The address of the share owner + * @return assets The amount of assets received + */ + function redeem(uint256 shares, address receiver, address owner) + public + override(ERC4626Upgradeable, IERC4626) + returns (uint256) + { + _checkDelegationConstraint(owner, shares); + return super.redeem(shares, receiver, owner); + } + + /** + * @notice Internal function to check if a share burn would violate delegation constraints + * @param owner The address of the share owner + * @param shares The amount of shares being burned + */ + function _checkDelegationConstraint(address owner, uint256 shares) internal view { + uint256 ownerBalance = balanceOf(owner); + if (ownerBalance < shares) { + revert IChamber.InsufficientChamberBalance(); + } + if (ownerBalance - shares < totalAgentDelegations[owner]) { + revert IChamber.ExceedsDelegatedAmount(); + } + } + + /** + * @notice Returns the first delegation timestamp for an agent + * @param agent The address of the agent + * @return The timestamp of the agent's first delegation (0 if never delegated) + */ + function getAgentFirstDelegationTime(address agent) external view returns (uint256) { + return agentFirstDelegationTime[agent]; + } + /** * @notice Returns the next transaction ID (current nonce) * @return uint256 The next transaction ID that will be assigned @@ -682,6 +762,41 @@ contract Chamber is ERC4626Upgradeable, ReentrancyGuardUpgradeable, Board, Walle return super.getConfirmation(tokenId, nonce); } + /** + * @notice Returns the full details of a specific transaction including timestamp + * @param nonce The index of the transaction to retrieve + * @return executed Whether the transaction has been executed + * @return confirmations Number of confirmations + * @return target The target address + * @return value The ETH value + * @return data The calldata + * @return submittedAt The timestamp when the transaction was submitted + */ + function getTransactionFull(uint256 nonce) + public + view + override(IWallet, Wallet) + returns ( + bool executed, + uint8 confirmations, + address target, + uint256 value, + bytes memory data, + uint256 submittedAt + ) + { + return super.getTransactionFull(nonce); + } + + /** + * @notice Checks if a transaction has expired + * @param nonce The index of the transaction to check + * @return True if the transaction has expired, false otherwise + */ + function isTransactionExpired(uint256 nonce) public view override(IWallet, Wallet) returns (bool) { + return super.isTransactionExpired(nonce); + } + /// @dev Storage gap for future upgrades uint256[50] private __gap; } diff --git a/src/Wallet.sol b/src/Wallet.sol index 7e3b0d4..21424d7 100644 --- a/src/Wallet.sol +++ b/src/Wallet.sol @@ -17,6 +17,7 @@ abstract contract Wallet { * @param target The destination address for the transaction * @param value The amount of ETH to send with the transaction * @param data The calldata to be executed + * @param submittedAt Timestamp when the transaction was submitted */ struct Transaction { bool executed; @@ -24,8 +25,12 @@ abstract contract Wallet { address target; uint256 value; bytes data; + uint256 submittedAt; } + /// @notice Transaction expiration period (30 days) + uint256 public constant TRANSACTION_EXPIRATION = 30 days; + /// @notice Array of all transactions submitted to the wallet Transaction[] internal transactions; @@ -56,6 +61,15 @@ abstract contract Wallet { _; } + /// @notice Modifier to check if a transaction has not expired + /// @param nonce The transaction index to check + modifier notExpired(uint256 nonce) { + if (block.timestamp > transactions[nonce].submittedAt + TRANSACTION_EXPIRATION) { + revert IWallet.TransactionExpired(); + } + _; + } + /** * @notice Submits a new transaction and auto-confirms for the submitter * @param tokenId The token ID submitting the transaction @@ -66,7 +80,16 @@ abstract contract Wallet { function _submitTransaction(uint256 tokenId, address target, uint256 value, bytes memory data) internal { uint256 nonce = transactions.length; - transactions.push(Transaction({target: target, value: value, data: data, executed: false, confirmations: 0})); + transactions.push( + Transaction({ + target: target, + value: value, + data: data, + executed: false, + confirmations: 0, + submittedAt: block.timestamp + }) + ); _confirmTransaction(tokenId, nonce); emit IWallet.SubmitTransaction(tokenId, nonce, target, value, data); } @@ -80,6 +103,7 @@ abstract contract Wallet { internal txExists(nonce) notExecuted(nonce) + notExpired(nonce) notConfirmed(tokenId, nonce) { Transaction storage transaction = transactions[nonce]; @@ -116,7 +140,12 @@ abstract contract Wallet { * @param tokenId The token ID executing the transaction * @param nonce The transaction index to execute */ - function _executeTransaction(uint256 tokenId, uint256 nonce) internal txExists(nonce) notExecuted(nonce) { + function _executeTransaction(uint256 tokenId, uint256 nonce) + internal + txExists(nonce) + notExecuted(nonce) + notExpired(nonce) + { Transaction storage transaction = transactions[nonce]; // Add zero address check @@ -169,6 +198,50 @@ abstract contract Wallet { (transaction.executed, transaction.confirmations, transaction.target, transaction.value, transaction.data); } + /** + * @notice Returns the full details of a specific transaction including timestamp + * @param nonce The index of the transaction to retrieve + * @return executed Whether the transaction has been executed + * @return confirmations Number of confirmations + * @return target The target address + * @return value The ETH value + * @return data The calldata + * @return submittedAt The timestamp when the transaction was submitted + */ + function getTransactionFull(uint256 nonce) + public + view + virtual + returns ( + bool executed, + uint8 confirmations, + address target, + uint256 value, + bytes memory data, + uint256 submittedAt + ) + { + Transaction storage transaction = transactions[nonce]; + return ( + transaction.executed, + transaction.confirmations, + transaction.target, + transaction.value, + transaction.data, + transaction.submittedAt + ); + } + + /** + * @notice Checks if a transaction has expired + * @param nonce The index of the transaction to check + * @return True if the transaction has expired, false otherwise + */ + function isTransactionExpired(uint256 nonce) public view virtual returns (bool) { + if (nonce >= transactions.length) revert IWallet.TransactionDoesNotExist(); + return block.timestamp > transactions[nonce].submittedAt + TRANSACTION_EXPIRATION; + } + /** * @notice Checks if a transaction is confirmed by a specific director * @param tokenId The tokenId of the director to check confirmation for diff --git a/src/interfaces/IChamber.sol b/src/interfaces/IChamber.sol index a3c28b5..a56cadb 100644 --- a/src/interfaces/IChamber.sol +++ b/src/interfaces/IChamber.sol @@ -63,6 +63,13 @@ interface IChamber is IERC4626, IBoard, IWallet { */ function getTotalAgentDelegations(address agent) external view returns (uint256); + /** + * @notice Returns the first delegation timestamp for an agent + * @param agent The address of the agent + * @return The timestamp of the agent's first delegation (0 if never delegated) + */ + function getAgentFirstDelegationTime(address agent) external view returns (uint256); + /** * @notice Updates the number of seats * @param tokenId The tokenId proposing the update @@ -216,4 +223,7 @@ interface IChamber is IERC4626, IBoard, IWallet { /// @notice Thrown when signature is invalid error InvalidSignature(); + + /// @notice Thrown when delegation is too recent to perform director actions + error DelegationTooRecent(); } diff --git a/src/interfaces/IWallet.sol b/src/interfaces/IWallet.sol index 101e279..4939b21 100644 --- a/src/interfaces/IWallet.sol +++ b/src/interfaces/IWallet.sol @@ -96,6 +96,35 @@ interface IWallet { */ function getNextTransactionId() external view returns (uint256); + /** + * @notice Returns the full details of a specific transaction including timestamp + * @param nonce The index of the transaction to retrieve + * @return executed Whether the transaction has been executed + * @return confirmations Number of confirmations + * @return target The target address + * @return value The ETH value + * @return data The calldata + * @return submittedAt The timestamp when the transaction was submitted + */ + function getTransactionFull(uint256 nonce) + external + view + returns ( + bool executed, + uint8 confirmations, + address target, + uint256 value, + bytes memory data, + uint256 submittedAt + ); + + /** + * @notice Checks if a transaction has expired + * @param nonce The index of the transaction to check + * @return True if the transaction has expired, false otherwise + */ + function isTransactionExpired(uint256 nonce) external view returns (bool); + /// Events /** * @notice Emitted when a new transaction is submitted @@ -149,4 +178,7 @@ interface IWallet { /// @notice Thrown when transaction target is invalid error InvalidTarget(); + + /// @notice Thrown when a transaction has expired (past 30 days) + error TransactionExpired(); } diff --git a/test/unit/Chamber.t.sol b/test/unit/Chamber.t.sol index c4d1ca6..f2dfa81 100644 --- a/test/unit/Chamber.t.sol +++ b/test/unit/Chamber.t.sol @@ -102,9 +102,13 @@ contract ChamberTest is Test { MockERC20(address(token)).approve(address(chamber), amount); chamber.deposit(amount, user1); chamber.delegate(tokenId, 1); + vm.stopPrank(); + + // Warp time to satisfy minimum delegation age + vm.warp(block.timestamp + 1 days + 1); + vm.prank(user1); chamber.submitTransaction(1, target, value, data); - vm.stopPrank(); (bool executed, uint8 confirmations, address trxTarget, uint256 trxValue, bytes memory trxData) = chamber.getTransaction(0); @@ -130,9 +134,13 @@ contract ChamberTest is Test { MockERC20(address(token)).approve(address(chamber), amount); chamber.deposit(amount, user1); chamber.delegate(tokenId, 1); + vm.stopPrank(); + + // Warp time to satisfy minimum delegation age + vm.warp(block.timestamp + 1 days + 1); + vm.prank(user1); chamber.submitTransaction(1, target, value, data); - vm.stopPrank(); (, uint8 confirmations,,,) = chamber.getTransaction(0); assertEq(confirmations, 1); @@ -152,7 +160,12 @@ contract ChamberTest is Test { MockERC20(address(token)).approve(address(chamber), amount); chamber.deposit(amount, user1); chamber.delegate(tokenId, 1); + vm.stopPrank(); + // Warp time to satisfy minimum delegation age + vm.warp(block.timestamp + 1 days + 1); + + vm.startPrank(user1); chamber.submitTransaction(1, target, value, data); chamber.revokeConfirmation(1, 0); vm.stopPrank(); @@ -184,26 +197,34 @@ contract ChamberTest is Test { MockERC20(address(token)).approve(address(chamber), amount); chamber.deposit(amount, user1); chamber.delegate(tokenId, 1); - chamber.submitTransaction(1, target, value, data); vm.stopPrank(); vm.startPrank(user2); MockERC20(address(token)).approve(address(chamber), amount); chamber.deposit(amount, user2); chamber.delegate(tokenId + 1, 1); - chamber.confirmTransaction(2, 0); vm.stopPrank(); vm.startPrank(user3); MockERC20(address(token)).approve(address(chamber), amount); chamber.deposit(amount, user3); chamber.delegate(tokenId + 2, 1); - chamber.confirmTransaction(3, 0); vm.stopPrank(); - vm.startPrank(user1); + // Warp time to satisfy minimum delegation age + vm.warp(block.timestamp + 1 days + 1); + + vm.prank(user1); + chamber.submitTransaction(1, target, value, data); + + vm.prank(user2); + chamber.confirmTransaction(2, 0); + + vm.prank(user3); + chamber.confirmTransaction(3, 0); + + vm.prank(user1); chamber.executeTransaction(1, 0); - vm.stopPrank(); (bool executed,,,,) = chamber.getTransaction(0); assertEq(executed, true); @@ -225,9 +246,13 @@ contract ChamberTest is Test { MockERC20(address(token)).approve(address(chamber), amount); chamber.deposit(amount, user1); chamber.delegate(tokenId, 1); + vm.stopPrank(); + + // Warp time to satisfy minimum delegation age + vm.warp(block.timestamp + 1 days + 1); + vm.prank(user1); chamber.submitTransaction(1, target, value, data); - vm.stopPrank(); uint256 count = chamber.getTransactionCount(); @@ -706,6 +731,9 @@ contract ChamberTest is Test { chamber.deposit(amount, user3); chamber.delegate(3, 1); vm.stopPrank(); + + // Warp time forward to satisfy minimum delegation age requirement + vm.warp(block.timestamp + 1 days + 1); } function test_Chamber_GetTotalAgentDelegations() public { @@ -1207,6 +1235,9 @@ contract ChamberTest is Test { chamber.delegate(6, 1); vm.stopPrank(); + // Warp time to satisfy minimum delegation age for new users + vm.warp(block.timestamp + 1 days + 1); + // user6 (tokenId 6) is the 6th but only 5 seats vm.prank(user6); vm.expectRevert(IChamber.NotDirector.selector); @@ -1380,6 +1411,202 @@ contract ChamberTest is Test { } function test_Chamber_Version() public view { - assertEq(chamber.version(), "1.1.3"); + assertEq(chamber.version(), "0.5"); + } + + // ============ New Security Feature Tests ============ + + function test_Chamber_DelegationTooRecent_Reverts() public { + uint256 tokenId = 1; + uint256 amount = 1 ether; + MockERC721(address(nft)).mintWithTokenId(user1, tokenId); + MockERC20(address(token)).mint(user1, amount); + + vm.startPrank(user1); + MockERC20(address(token)).approve(address(chamber), amount); + chamber.deposit(amount, user1); + chamber.delegate(tokenId, 1); + + // Try to submit transaction immediately (should fail) + vm.expectRevert(IChamber.DelegationTooRecent.selector); + chamber.submitTransaction(1, address(0x3), 0, ""); + vm.stopPrank(); + } + + function test_Chamber_DelegationAgeRequirement_Success() public { + uint256 tokenId = 1; + uint256 amount = 1 ether; + MockERC721(address(nft)).mintWithTokenId(user1, tokenId); + MockERC20(address(token)).mint(user1, amount); + + vm.startPrank(user1); + MockERC20(address(token)).approve(address(chamber), amount); + chamber.deposit(amount, user1); + chamber.delegate(tokenId, 1); + vm.stopPrank(); + + // Warp time forward past minimum delegation age + vm.warp(block.timestamp + 1 days + 1); + + // Now should succeed + vm.prank(user1); + chamber.submitTransaction(1, address(0x3), 0, ""); + + assertEq(chamber.getTransactionCount(), 1); + } + + function test_Chamber_GetAgentFirstDelegationTime() public { + uint256 tokenId = 1; + uint256 amount = 100; + MockERC721(address(nft)).mintWithTokenId(user1, tokenId); + MockERC20(address(token)).mint(user1, amount); + + // Before delegation, should be 0 + assertEq(chamber.getAgentFirstDelegationTime(user1), 0); + + vm.startPrank(user1); + token.approve(address(chamber), amount); + chamber.deposit(amount, user1); + + uint256 delegationTime = block.timestamp; + chamber.delegate(tokenId, amount); + vm.stopPrank(); + + // After delegation, should be set + assertEq(chamber.getAgentFirstDelegationTime(user1), delegationTime); + } + + function test_Chamber_Withdraw_ExceedsDelegatedAmount_Reverts() public { + uint256 tokenId = 1; + uint256 amount = 1000; + MockERC721(address(nft)).mintWithTokenId(user1, tokenId); + MockERC20(address(token)).mint(user1, amount); + + vm.startPrank(user1); + token.approve(address(chamber), amount); + chamber.deposit(amount, user1); + chamber.delegate(tokenId, amount); + + // Try to withdraw - should fail because all shares are delegated + vm.expectRevert(IChamber.ExceedsDelegatedAmount.selector); + chamber.withdraw(amount, user1, user1); + vm.stopPrank(); + } + + function test_Chamber_Redeem_ExceedsDelegatedAmount_Reverts() public { + uint256 tokenId = 1; + uint256 amount = 1000; + MockERC721(address(nft)).mintWithTokenId(user1, tokenId); + MockERC20(address(token)).mint(user1, amount); + + vm.startPrank(user1); + token.approve(address(chamber), amount); + chamber.deposit(amount, user1); + chamber.delegate(tokenId, amount); + + // Try to redeem - should fail because all shares are delegated + vm.expectRevert(IChamber.ExceedsDelegatedAmount.selector); + chamber.redeem(amount, user1, user1); + vm.stopPrank(); + } + + function test_Chamber_Withdraw_PartialDelegation_Success() public { + uint256 tokenId = 1; + uint256 amount = 1000; + uint256 delegateAmount = 500; + MockERC721(address(nft)).mintWithTokenId(user1, tokenId); + MockERC20(address(token)).mint(user1, amount); + + vm.startPrank(user1); + token.approve(address(chamber), amount); + chamber.deposit(amount, user1); + chamber.delegate(tokenId, delegateAmount); + + // Should be able to withdraw undelegated portion + uint256 withdrawAmount = amount - delegateAmount; + chamber.withdraw(withdrawAmount, user1, user1); + vm.stopPrank(); + + assertEq(token.balanceOf(user1), withdrawAmount); + } + + function test_Chamber_TransactionExpiration() public { + addDirectors(); + + vm.prank(user1); + chamber.submitTransaction(1, address(0x3), 0, ""); + + // Check transaction is not expired initially + assertFalse(chamber.isTransactionExpired(0)); + + // Warp forward past expiration (30 days) + vm.warp(block.timestamp + 31 days); + + // Check transaction is expired + assertTrue(chamber.isTransactionExpired(0)); + } + + function test_Chamber_ConfirmExpiredTransaction_Reverts() public { + addDirectors(); + + vm.prank(user1); + chamber.submitTransaction(1, address(0x3), 0, ""); + + // Warp forward past expiration + vm.warp(block.timestamp + 31 days); + + // Try to confirm expired transaction + vm.prank(user2); + vm.expectRevert(); + chamber.confirmTransaction(2, 0); + } + + function test_Chamber_ExecuteExpiredTransaction_Reverts() public { + addDirectors(); + + vm.prank(user1); + chamber.submitTransaction(1, address(0x3), 0, ""); + + vm.prank(user2); + chamber.confirmTransaction(2, 0); + + vm.prank(user3); + chamber.confirmTransaction(3, 0); + + // Warp forward past expiration + vm.warp(block.timestamp + 31 days); + + // Try to execute expired transaction + vm.prank(user1); + vm.expectRevert(); + chamber.executeTransaction(1, 0); + } + + function test_Chamber_GetTransactionFull() public { + addDirectors(); + + uint256 beforeSubmit = block.timestamp; + vm.prank(user1); + chamber.submitTransaction(1, address(0x3), 0, ""); + + ( + bool executed, + uint8 confirmations, + address target, + uint256 value, + bytes memory data, + uint256 submittedAt + ) = chamber.getTransactionFull(0); + + assertEq(executed, false); + assertEq(confirmations, 1); + assertEq(target, address(0x3)); + assertEq(value, 0); + assertEq(data, ""); + assertGe(submittedAt, beforeSubmit); + } + + function test_Chamber_MinimumDelegationAge_Constant() public view { + assertEq(chamber.MINIMUM_DELEGATION_AGE(), 1 days); } } diff --git a/test/unit/ChamberUpgrade.t.sol b/test/unit/ChamberUpgrade.t.sol index fcb5130..c432c4c 100644 --- a/test/unit/ChamberUpgrade.t.sol +++ b/test/unit/ChamberUpgrade.t.sol @@ -82,6 +82,9 @@ contract ChamberUpgradeTest is Test { chamber.deposit(amount, user3); chamber.delegate(3, amount); vm.stopPrank(); + + // Warp time to satisfy minimum delegation age requirement + vm.warp(block.timestamp + 1 days + 1); } function test_Chamber_GetProxyAdmin() public view { @@ -179,9 +182,10 @@ contract ChamberUpgradeTest is Test { bytes memory upgradeData = abi.encodeWithSelector(IChamber.upgradeImplementation.selector, address(newImplementation), ""); - // Non-director cannot submit upgrade transaction (tokenId 999 not in top seats) + // Non-director cannot submit upgrade transaction + // Since they never delegated, DelegationTooRecent is thrown first vm.prank(nonDirector); - vm.expectRevert(IChamber.NotDirector.selector); + vm.expectRevert(IChamber.DelegationTooRecent.selector); chamber.submitTransaction(999, chamberAddress, 0, upgradeData); }