| Issue | Unused Constant Definitions Creates Dead Code |
|---|---|
| Location | Location: sources/poel/iAsset.move, lines 53, 59, 65, 74, 77, 85, 106 |
| Severity | Informational |
| Description | The following constants are defined in iAsset.move but are never referenced anywhere in the codebase: |
const EWRONG_CWV_LENGTH: u64 = 8; const EWRONG_DESIRABILITY_SCORE_LEN: u64 = 10; const EWRONG_ASSET_COUNT: u64 = 12; const ENOT_ENOUGH_TIME_PASSED: u64 = 16; const ENOT_ENOUGH_WITHDRAWABLE_REWARDS: u64 = 17; const INVALID_TS: u64 = 20; const MAX_U64: u64 = 18446744073709551615; | | Recommendations | Remove all unused constant definitions from iAsset.move: • EWRONG_CWV_LENGTH • EWRONG_DESIRABILITY_SCORE_LEN • EWRONG_ASSET_COUNT • ENOT_ENOUGH_TIME_PASSED • ENOT_ENOUGH_WITHDRAWABLE_REWARDS • INVALID_TS • MAX_U64 | | Comments / Resolution | Acknowledged |
| Issue | Error Code Numbering Inconsistencies |
|---|---|
| Location | Location: sources/poel/iAsset.move, lines 53, 59, 65, 74, 77, 85, 106 |
| Severity | Informational |
| Description | There are inconsistencies in the definition and assignment of error code numbers across the Move codebase. These include duplicate error codes within the same module, gaps in error code sequences, and potential cross-module confusion due to reused error code numbers. |
| Issue | Redundant Immutable Borrow Prevents Mutable Borrow of RewardBudget in distribute_rewards |
|---|---|
| Location | Location: sources/poel/poel.move, function: distribute_rewards, line 831 |
| Severity | Informational |
| Description | An unused immutable borrow of the RewardBudget global resource is performed on line 831: |
| let reward_ref = borrow_global<RewardBudget>(get_storage_address()); | |
| Later in the same function (line 839), a mutable borrow of the same RewardBudget resource is performed: | |
| let reward_budget_ref = borrow_global_mut<RewardBudget>(obj_address); | |
| Both borrows reference the same address (get_storage_address()), either directly or via the obj_address variable. The Move borrow checker does not permit both an immutable and a mutable borrow of the same resource at the same time, even if the immutable borrow is unused. This results in a compilation error. | |
| Recommendations | Remove the unused immutable borrow on line 831. |
| Comments / Resolution | Acknowledged |
| Issue | Unused Functions |
|---|---|
| Location | Location: |
| • sources/asset_pool/asset_util.move: get_address_key, get_supra_coin_key | |
| • sources/poel/iAsset.move: deconstruct_asset_entry (lines 879–881) | |
| Severity | Informational |
| Description | The functions get_address_key and get_supra_coin_key in asset_util.move, and deconstruct_asset_entry in iAsset.move, are defined but not used anywhere in the codebase. Their presence may indicate planned future use, a leftover from refactoring, or an intended external API that is not yet integrated. |
| Recommendations | • If the function is intended for future use or as a public API, document its purpose clearly in the code comments. |
| • If it is not needed, consider removing the function to keep the codebase clean and maintainable. | |
| • If it is meant for external module or testing use, ensure it is properly tested and referenced in documentation. | |
| Comments / Resolution | Acknowledged |
| Issue | Frozen Accounts Can Still Receive Minted iAssets |
|---|---|
| Location | Location: sources/poel/iAsset.move, function: mint_iasset(), lines 920–955 (esp. line 950) |
| Severity | Medium |
| Description | The mint_iasset() function does not check if the target account is frozen before minting iAssets. This allows frozen accounts to receive new tokens, even though they cannot transfer existing balances, leading to inconsistent security controls. |
| Recommendations | Add an account freeze check before minting. Define an appropriate error constant (e.g., EACCOUNT_FROZEN). |
| Comments / Resolution |
| Issue | Division by Zero in APY Calculation Due to Unvalidated Epoch Length |
|---|---|
| Location | Location |
| • Primary: sources/poel/iAsset.move:1805 in calculate_asset_apy() function | |
| • Root Cause: sources/config/config.move:313 (missing validation in parameter setting) | |
| Severity | Low |
| Description | The calculate_asset_apy function contains a division by zero vulnerability that occurs when config::get_length_of_epoch() returns 0. At line 1805, the function calculates let number_of_epoch_year = ONE_YEAR_SEC / config::get_length_of_epoch() without validating that the epoch length is non-zero. The MutableParameters.length_of_epoch field is initialized to 0 during contract deployment and can only be updated through the set_parameters function, which lacks validation to prevent administrators from setting invalid values. |
| This vulnerability manifests in two scenarios: initially when the protocol deploys with length_of_epoch = 0 and any APY calculation fails until properly configured, or when an administrator accidentally sets length_of_epoch = 0 during parameter updates. When triggered, this causes all reward allocation operations and APY calculations to fail with runtime aborts, effectively creating a denial of service for core protocol functionality. While no funds are at risk and the issue can be immediately resolved by calling set_parameters with a valid epoch length, the dependency on administrator action and the disruption to essential protocol operations justifies medium severity classification. | |
| Recommendations | Add validation in the set_parameters function: |
| assert!(length_of_epoch > 0, error::invalid_argument(EINVALID_EPOCH_LENGTH)); | |
| Additional Recommendations: | |
| Defensive Programming: Add a check in calculate_asset_apy before division: | |
| let epoch_length = config::get_length_of_epoch(); | |
| assert!(epoch_length > 0, error::invalid_state(EINVALID_EPOCH_CONFIG)); | |
| let number_of_epoch_year = ONE_YEAR_SEC / epoch_length; | |
| Comments / Resolution |
| Issue | Inconsistent Struct Reference Management in allocate_rewards |
|---|---|
| Location | • File: sources/poel/poel.move |
| • Function: allocate_rewards (lines 739-820) | |
| • Lines: 757, 778, 779, 791 | |
| Severity | Low |
| Description | The allocate_rewards function exhibits inconsistent and confusing patterns in managing struct references, leading to code that is difficult to maintain and prone to developer errors: |
| Variable Shadowing with Different Borrow Types: | |
| // Line 757: Immutable borrow | |
| let delegated_amount_ref = borrow_global<DelegatedAmount>(obj_address); |
// Line 778: Mutable borrow shadows the immutable reference let delegated_amount_ref = borrow_global_mut<DelegatedAmount>(obj_address); // Line 779: First mutable borrow let reward_budget_ref = borrow_global_mut<RewardBudget>(obj_address); // Line 791: Immediately re-borrow the same resource let reward_budget_ref = borrow_global_mut<RewardBudget>(obj_address); The function switches between immutable and mutable references to the same resources without clear separation, making it unclear which reference capabilities are needed at each point. | | Recommendations | Use Distinct Variable Names: // Immutable references for calculations let delegated_amount_read = borrow_global<DelegatedAmount>(obj_address); let reward_budget_read = borrow_global<RewardBudget>(obj_address); // Mutable references for updates let delegated_amount_mut = borrow_global_mut<DelegatedAmount>(obj_address); let reward_budget_mut = borrow_global_mut<RewardBudget>(obj_address); | | Comments / Resolution | |
| Issue | Division by Zero Risk in Collateralization Rate Calculation |
|---|---|
| Location | Location: sources/poel/iAsset.move, lines 53, 59, 65, 74, 77, 85, 106 |
| Severity | Low |
| Description | The calculate_collateralization_rate function in the iAsset.move module performs division operations using the desired_weight variable as a denominator. If desired_weight is set to 0 (the default for new assets) or to MAX_WEIGHT (100,000), this will result in a division by zero, causing the transaction to abort unexpectedly. This can occur immediately after asset creation (since the default is zero) or if an administrator sets an invalid weight via the batch update function. This makes the system fragile and prone to runtime errors if not carefully managed. |
| Recommendations | • Initialization: Set a safe, non-zero default value for desired_weight when creating new assets (e.g., desired_weight: 1). |
| • Validation: Add assertions in the batch_update_desired_weight function to ensure that desired_weight is always greater than 0 and less than MAX_WEIGHT. | |
| Comments / Resolution |
| Issue | Admin Accounting Updates Lack Validation, Risk of Accidental Protocol Insolvency |
|---|---|
| Location | File: sources/poel/poel.move |
| Function: update_accounting_states (lines 389-403) | |
| Line: 399 - delegated_amount_ref.total_delegated_amount = total_delegated_amount; | |
| Severity | Low |
| Description | The update_accounting_states function allows administrators to manually update total_delegated_amount without any validation against actual on-chain state. This creates significant risk for accidental protocol damage through human error. |
| Since reward calculations depend on this value: | |
| Apply to poel.move | |
| total_delegated_amount | |
| Accidental scenarios that could break the protocol: | |
| Typo/Input Error: | |
| Actual staked: 1,000,000 SUPRA | |
| Admin meant: 1,000,000 SUPRA | |
| Admin typed: 100,000 SUPRA (missing zero) | |
| Result: 900,000 "fake rewards" → Protocol insolvency | |
| Wrong Units: | |
| Admin thinks in millions: enters 1 (meaning 1M) | |
| System interprets as: 1 SUPRA | |
| Result: Nearly all staked funds treated as "rewards" | |
| Recommendations | If this function is to be kept, then perhaps add validation such as: |
let actual_staked = get_total_staked() + borrow_global<ReplacedDelegationPool>(get_storage_address()).delegated; let min_allowed = actual_staked * 80 / 100; // Allow 20% below actual let max_allowed = actual_staked * 120 / 100; // Allow 20% above actual assert!(total_delegated_amount >= min_allowed && total_delegated_amount <= max_allowed, error::invalid_argument(EINVALID_ACCOUNTING_VALUE)); // ... rest of function | | Comments / Resolution | |
| Issue | Misuse of #[view] Annotation in get_fa_pool_details |
|---|---|
| Location | File: sources/asset_pool/asset_pool.move |
| • Function: get_fa_pool_details | |
| • Line: 138 | |
| Severity | Low |
| Description | The function get_fa_pool_details is annotated with #[view], which signals to off-chain clients and tooling (e.g., aptos move view, REST API queries, or SDKs) that the function is read-only. However, the function currently uses borrow_global_mut: |
| let pool = borrow_global_mut<AssetPoolVault>(get_storage_address()); | |
| While this doesn't break on-chain execution, it violates the semantics of a view function, which is expected to make no mutations or acquire mutable references. Using borrow_global_mut in a function marked #[view] contradicts its intended semantics and may confuse developers or break future assumptions in tooling or audits. | |
| Recommendations | Change the function public fun get_fa_pool_details: |
| let pool = borrow_global_mut<AssetPoolVault>(get_storage_address()); | |
| To | |
| let pool = borrow_global<AssetPoolVault>(get_storage_address()); | |
| Comments / Resolution | Acknowledged |
| Issue | Incentive Inversion in Collateralization Rate Calculation Causes Protocol Imbalance |
|---|---|
| Location | File: sources/poel/iAsset.move |
| • Function: calculate_collateralization_rate | |
| • Lines: 1336–1346 (underweight asset branch) | |
| Severity | High |
| Descript | |
| ion | The withdraw_surplus_rewards function contains a logic error in its assertion that validates surplus reward withdrawals. The assertion incorrectly adds total_borrowed_amount (funds already lent out) back to the vault balance, creating a false impression of available funds: |
| assert!(balance + delegated_amount_ref.total_borrowed_amount - requested >= delegated_amount_ref.total_borrowable_amount + reward_budget_ref.allocated_reward_balance, error::invalid_state(EAMOUNT_EXCEEDED)); | |
| The total_borrowed_amount represents funds that have already been borrowed out of the vault and are no longer available. Adding these funds back to the balance calculation allows withdrawal of funds that should be reserved for future borrowing capacity and allocated rewards. This can lead to protocol undercollateralization where the vault lacks sufficient funds to honor borrowing commitments and reward distributions. | |
| Recommendations | Change the assertion: |
| assert!(balance + delegated_amount_ref.total_borrowed_amount - requested >= delegated_amount_ref.total_borrowable_amount + reward_budget_ref.allocated_reward_balance, error::invalid_state(EAMOUNT_EXCEEDED)); | |
| To: | |
| assert!(balance - requested >= (delegated_amount_ref.total_borrowable_amount - delegated_amount_ref.total_borrowed_amount) + reward_budget_ref.allocated_reward_balance, error::invalid_state(EAMOUNT_EXCEEDED)); | |
| Comments / Resolution | Fixed |
| Issue | Decimal Mismatch in Redemption Leads to Incorrect FA Withdrawal Amounts |
|---|---|
| Location | Location:File: sources/poel/poel.moveFunction: redeem_iassetLines: 1455–1459Related helper:File: sources/asset_pool/asset_util.moveFunction: scale_to_originLines: 44–46 |
| Severity | High |
| Description | The redemption path computes amount in 8-decimal iAsset units and then “de-normalizes” it using origin_token_decimals before passing it to the withdrawal router. However, the router withdraws from the FA as represented on this chain, whose Metadata-defined decimals may differ from the origin chain’s decimals. |
| If origin_token_decimals ≠ on-chain FA decimals(asset), the scaled n_amount will not match the units expected by asset_pool::withdraw_fa. | |
| Example: For USDC with 6 origin decimals but 8 decimals on this chain, scale_to_origin converts 8→6 (divides by 100). The router then uses this 6-decimal-sized value against an 8-decimal FA, causing a 100x under-withdrawal. The reverse configuration can cause over-withdrawal. | |
| Recommendations | Compute the final scale using the FA’s actual on-chain decimals instead of origin chain decimals. |
| Two safe approaches: |