suiUSDe

The replenish function does not validate the active status of a collateral that is being replenished. Because of this it might be possible for any caller to accidentally replenish a disabled collateral.

Consider validating that the collateral is enabled in the replenish function.

Nonces in the Move implementation are arbitrary strings with no distinct limit on their length.

This means that they could be used to inject malicious code into whatever systems are consuming the mint/redeem events that emit this nonce value.

Consider adding a reasonable length validation on the nonce string to decrease the possibility of them being able to be used as malicious injection.

When creating the suiUSDe token in the init_treasury function the make_regulated function receives an allow_global_pause value of true which indicates that the protocol would like to have the ability to pause usage of the suiUSDe coin across the entire network with the deny_list_v2_enable_global_pause function.

However in the denylist_manager module there are only functions exposed to add addresses and remove addresses from the denylist. None that expose the ability to pause and unpause the global pause functionality which has been whitelisted for the suiUSDe token.

Consider exposing the global pause and unpause functionality through the deny_list_v2_enable_global_pause function if it is desired to have this capability.

The new_oracle_proof and commit_pyth_price functions do not implement a config version check from the Treasury, and thus are callable even after a package upgrade has occurred and the version has been bumped.

If important updates were made to the Oracle in the upgrade, as long as no new logic has been added in the upgrade that prevents the old Oracle logic from being used, then the old oracle logic can be used to create unexpected OracleProof objects which could contain prices that do not meet the standards of the new version of the contracts.

Enforce the config version check in the new_oracle_proof and commit_pyth_price functions.

In the commit_pyth_oracle_price flow it is possible for a benefactor user to control the end outcome of the median result and potentially arbitrage the system when multiple oracle feeds are supported because of a few factors.

The commit_pyth_oracle_price function will commit “invalid” price types to the proof to indicate that the price was provided for the relevant feed but was found to be invalid. The validate_and_get_valid_prices function which retrieves the valid prices on usage tosses out any entries that were deemed to be invalid upon commitment.

The user can control whether a price is considered valid on commitment by choosing to pass a stale Pyth update versus passing an up to date one.

This means a benefactor could provide a commitment such as the following:

• Assume minimum_sources is assigned as 2 and there are 3 total feeds configured
• For feed 0 (A pyth feed) the value is $1.01, the user provides a price that is 5 minutes old, it is considered invalid
• For feed 1 the value is $1.00, and the price is valid
• For feed 2 the value is $0.99, and the price is valid
• The computed median is the average of feed 1 and feed 2 which is $0.995

The benefactor could mint using this price, and then redeem using a different price they commit:

• For feed 0 (A pyth feed) the value is $1.01, the user now provides a price that is only 10 seconds old, it is considered valid
• For feed 1 the value is $1.00, and the price is valid
• For feed 2 the value is $0.99, and the price is valid
• The computed median is now feed 1, which is $1.00
• If the spread between these two median results is large enough, the user could gain an immediate arbitrage on the system

When multiple feeds are supported by the aggregate oracle, consider requiring that all pull based feeds where the user provides their own price update have valid staleness.

At the time of price usage, in the latest_price function, the earliest price from all oracle sources is compared against the collateral_limits maximum age configuration.

However this is a different configuration than the oracle.limits.max_age_ms. Currently these are to be configured to the same value, but in the event that they are not configured to the same value this may allow prices to be committed in a fresh state and then become stale by the aggregate oracle’s standards by the time the proof is used.

More notably, when multiple oracle feeds are supported the collateral_limits max staleness will not be able to apply to each feed individually and so therefore must take the maximum staleness value of all of the feeds. Therefore if there is an onchain aggregator in the list of supported feeds, which has a heartbeat of 12 hours, then the collateral_limits max staleness must be configured to at least 12 hours.

This is far too long for a pyth push based feed, and would allow a user to knowingly or unknowingly commit a pyth push price and wait up to 12 hours before using it, at which point it can be notably stale and out of line with the current market values.

In the future when multiple feeds are supported by the Aggregate Oracle, also perform per-feed level validations on staleness at usage time in the latest_price function.

The Aggregated Oracle is intended to function with multiple feed sources and compute a median result of those provided. However the validation on staleness is only applied at the aggregate oracle level.

When multiple feeds are supported they may have different staleness tolerance requirements and thus should have a feed level staleness validation.

While the existing setup only uses a single pyth feed, in the future when an upgrade is made to support multiple feeds, an individual feed level staleness validation should be introduced as a broader aggregate feed validation may not be able to be strict enough for some feeds in the aggregate oracle.

In the validate_states function the benefactor_config.assert_is_enabled method is invoked to validate that the benefactor is enabled, however the following benefactor_config.add_nonce call already includes such validation in its function body. Therefore the previous bespoke invocation is unnecessary.

Consider removing the benefactor_config.assert_is_enabled inside the validate_states function as this is already performed within the add_nonce invocation.

Throughout the codebase Committed is misspelled as Commited.

Correct these instances to Committed.

In the treasury.move file the create function for the Treasury object does not validate that the config provided uses an up to date version.

As a result, a treasury can be errantly created that is immediately invalid and outdated.

Consider adding validation that the config provided to the create function is using an up to date config version.

The validate() call in set_minimum_sources revalidates oracle.limits, even though limits aren’t changed there. Limits are already validated at creation and when setters are used, so this check is redundant in this path.

Remove this oracle limits validation in set_minimum_sources path

In the oracle_limits module within the suiusde package, the validate function which is used to check the limit configurations allows the max_price to be equal to the min_price.

This however is unlikely to be a realistic valid configuration and does not align with the validations made on the Solidity implementation.

Consider enforcing more strict validations that require the max_price to be strictly greater than the min_price.

In the mint and redeem functions for the benefactor, the fee_is_charged value is intended to indicate whether the collateralization of the action was better than a direct oracle exchange for the protocol, e.g. that the fee was charged.

However the fee_is_charged value is also assigned to true when the one_to_one_amount_out is equivalent to the oracle_amount_out. When this is the case no fee was not charged relative to the oracle price, so the fee_is_charged value should instead be false to indicate this.

Consider updating the fee_is_charged definition to be a strict less than comparison, so that it is only true when some nonzero fee is charged between the one_to_one_amount_out and oracle_amount_out.

In the Solidity implementation of the stablecoin system, there is validation which prevents the user from providing a minAmountOut of 0 accidentally. However the Move implementation does not implement such a validation.

Consider if the minimum amount out should be required to be nonzero to protect against potential misconfigurations.

The set_version function in the admin module allows setting any values as version, including older or current versions. This can lead to unintended downgrades or redundant updates, potentially causing compatibility or versioning issues.

In set_version function only allow strictly increasing values to version to prevent accidental or malicious downgrades and redundant updates

In the add_to_denylist function the treasury's deny_cap_mut function and other similar functions which validate the config version against the current breaking version is used.

This means that when in the middle of an upgrade process, or generally when the contracts for the minting system have been temporarily disabled, the denylist is not able to be updated.

This may create a risk of a lack of ability to respond to an incident if it occurs during this temporary period, or may create operational issues with the ability to adhere to compliance of KYC etc... during this period.

Furthermore, the add_to_denylist requires a &Auth reference value which can only be obtained by the new_auth function which also performs the assert_valid_version validation.

Consider implementing and using versions of the has_benefactor, benefactor_mut, and deny_cap_mut functions that do not validate the config version in order to support blacklisting addresses in emergency scenarios even when the minting system is temporarily disabled with the add_to_denylist function.

Furthermore, consider using the following authentication check in the add_to_denylist function instead of a typed check:

treasury.roles_for_version_update().assert_is_authorized<DenylistManagerRole>(ctx.sender());

In the EVM implementation of the stablecoin system the oracle admin cannot accidentally brick the oracle by assigning the minimum oracle sources to be above the available number of oracles in the aggregate oracle.

However in the Move implementation there is no such validation that prevents the oracle manager from assigning a minimum_sources value that is above the number of supported feeds.

Furthermore, in the remove_feed function there is no validation that requires that the number of remaining oracle feeds exceeds the minimum. This also deviates from the EVM implementation.

Consider if these validations should be implemented to be in line with the EVM implementation.

In the EVM implementation of the minting system Benefactor configurations such as mint and redeem fees, benefactor limits, and fee exemption statuses can be configured by the benefactor manager before a benefactor address has been whitelisted as a benefactor.

This is however not the case in the Move implementation as the benefactor_mut function requires that the address is contained in the treasury.benefactors Bag.

This is a fine behavior, however deviates from the EVM implementation. For any integrators who are supporting and working with the EVM implementation this may be unexpected.

Consider documenting this behavior, or at least be aware of this discrepancy compared to the EVM version.

In the EVM implementation the benefactor manager configuration functions are careful to only emit events that indicate a successful configuration when the newly configured value is different from the one that existed before it.

The Move implementation however does not do this and always emits the relevant event even if the configuration did not change anything.

Be aware of this discrepancy between the EVM and Move implementations.

In the calculate_usd_price_internal function the oracle buffer validation requires that: constants::oracle_buffer() + target_decimals > oracle_decimals with strict inequality.

However the equality case is a valid scenario as well which does not cause an underflow when calculating the exponent_with_buffer result, though perhaps unlikely in real world configurations.

Consider using a greater than or equal to comparison for the validation to allow the full range of technically valid configurations.

Since Pyth prices originate from offchain it is entirely possible that the oracle timestamp is in the future relative to the trusted Sui clock.

The Solidity version accounts for this by validating that prices are no more than 15 seconds into the future, as anything further would be unexpected and likely point to some mis-configuration.

Consider adding a future timestamp threshold, whereby if a Pyth price result is from too far in the future than would be normally realistic for time mismatch between Pyth’s offchain system and Sui’s trusted clock the the transaction is aborted.

In the new_auth function the treasury parameter is passed as a mutable reference, however it is not used in a way that requires mutability.

Mark the treasury variable as an immutable parameter.

In the EVM implementation the oracle feed contracts used in the aggregate oracle contract scale the price result up to 18 decimals for an internal price representation that has 18 decimals of precision.

However the oracle implementation in the Move implementation scales price results (often times down) to 6 decimals. For example, the pyth feed for USDC has 8 decimals of price precision, which would be scaled down to 6, thus destroying some precision.

Consider using a higher magnitude scale such as 8 decimals instead of down to 6 for the internal price representation to preserve as much price granularity as possible.

In the collateral_to_suiusde_amount function there is no validation that prevents the final suiusde_amount result from overflowing the u64 cast that is made.

This scenario is certainly invalid as the Coin object has a value field of u64 and thus the totalSupply cannot exceed this.

However it may technically be possible to reach this state if a certain collateral with lower decimals is used in tandem with the peg price being set extremely low.

Consider the following case:

• suiusde_decimals = 6
• collateral_usd_price = 1e6
• collateral_amount = 20_000_000e6
• collateral decimals = 6
• suiusde_peg_price = 1
• numerator = 20_000_000e6 * 1e6 * 10**6 = 20e24
• denominator = 1e6 * 1 = 1e6
• suiusde_amount = 20e24 / 1e6 = 20e18 > type(u64) maximum

Assigning the suiusde_peg_price to 1 is unlikely to ever happen and would be considered an mis-configuration in almost all scenarios. However it does produce this overflow case. Furthermore, unusual tokens with less than 6 decimals would increase the possible peg price assignments which can lead to an overflow.

Consider validating that the numerator / denominator result is not larger than the maximum u64 as a safety check in the collateral_to_suiusde_amount function. Furthermore, consider validating that the peg price is above some reasonable minimum which would also prevent this possibility.

The maximum peg price validation allows a peg price of up to 1,000 with six decimal places. However under certain circumstances, for collateral tokens that have higher decimals such as 9 decimals, matching that of the Sui token, an overflow case can still occur in the suiusde_to_collateral_amount function.

Consider the following scenario:

• suiusde_peg_price = 1_000e6
• suiusde_amount = 20_000_000e6
• collateral_decimals = 1e9
• collateral_usd_price = 1e6
• suiusde_decimals = 1e6
• numerator = 1_000e6 * 20_000_000e6 * 1e9 = 20e30
• denominator = 1e6 * 1e6 = 1e12
• collateral_amount = 20e30 / 1e12 = 20e18 > type(u64) maximum

This scenario is unlikely to ever occur, but still shows an overflow with the maximum allowable peg price in place.

Consider lowering the maximum allowable peg price to a more restrictive value. And furthermore consider adding an overflow check in the suiusde_to_collateral_amount just as a safety precaution to prevent any unexpected scenarios.

In the mint and redeem functions the fee_is_charged value is a boolean which is set to true whenever the one_to_one_amount, which includes a fee, is less than the oracle_amount_out.

The fee_is_charged value is included in the emit_order_executed event emission directly after the fee_bps value, which may lead consumers to believe that the entire bps of the order amount were taken as a fee.

However, in the event that fee_is_charged is true it just means that some fee was charged relative to the oracle priced valuation, not that the entire bps was charged relative to the oracle valuation.

For example:

• Collateral of order = 10 USDC
• Fee = 3%
• Oracle valuation of order collateral = $9.90
• One to one valuation of order collateral with fee = $9.70
• The technical fee levied was only 200 BPS relative to the oracle valuation

Be aware of this inconsistency and consider emitting the actual delta from the oracle valuation as the fee in the event. Otherwise document this for consumers.

In the new_proof function the oracle parameter is passed by mutable reference, however the contents of the function do not use the value mutably and it can instead be an immutable reference parameter.

Since the parameter is currently marked as mutable, it must be ordered with other mint and redeem actions which necessarily also require a mutable reference to the oracle through the proof creation process as well as any malicious transactions which simply touch the permissionless new_oracle_proof function. In some cases through high usage or malicious flow, this may cause unnecessary congestion for minting and redeeming actions.

Make the oracle parameter a immutable reference instead of a mutable one.

Given that the Pyth feed is a push based one where the exact price that is used is controlled by the publisher, within the staleness threshold enforced, the protocol should carefully consider the staleness threshold with respect to the volatility of the collateral being configured.

For example, if a staleness period of 300 seconds or 5 minutes is used, then some assets may realistically move 1% in that timeframe on a semi-regular instance. If the price of a collateral token at t = 0 was $1.01 while it is now at t = 300 at $1.00 this may allow for an arbitrage opportunity depending on the fee configurations of the protocol.

For example:

• Benefactor A uses the price update from t = 0 to mint with their collateral token being valued at $1.01, it is within the staleness tolerance of 300 seconds
• Benefactor A then uses the latest price from the current t = 300 to redeem with the same collateral token they are redeeming now being valued at only $1.00
• As long as the fees do not overcome the 1% delta then the Benefactor A stands to make a risk free profit

Consider the staleness threshold for each asset carefully in accordance with the collateral volatility and the corresponding fees being used, also keeping in mind that some benefactors may have little fees with a custom fee or no fees with an exemption.

There is a subtle difference between the EVM implementation and the Move implementation with regards to the changing of duration of epochs and periods and the maintaining of the previous duration’s counter values.

In the EVM implementation the epoch and period states are not overwritten, as they are keyed based on the duration length. Instead when a new duration is assigned the epoch/period state is pointing to an entirely new storage slot while the old duration’s storage slot remains intact.

In the Move implementation there is only one object that stores the current state of the counter for the epoch or period. This means that when the duration is changed the counter values for the epoch and period are overwritten when the relevant mint/redeem action touches this limit.

This subtle difference only manifests itself when a period/epoch duration is changed from A to B and back to A again while still within the same sequence number. In the EVM implementation the previous counter would still hold the original values, in the Move implementation they have been reset to zero.

This behavior also presents itself where some epoch/period values may have been reset to zero while others have not been. This is because counters are only rolled over/reset when they are interacted with. So the global counter is likely to be rolled over in this case, but it may be the case that a benefactor counter or particular collateral counter are not reset since a mint/redeem action may not have happened with them while the package was using the updated B duration.

Such a configuration scenario is unlikely to arise, simply be aware of this difference between the Move and EVM implementations.

The comment above the validate_states function suggests that the order timestamp validation is performed as a part of the validate_states execution. However this validation is not performed until the end of the mint/redeem action when order.finalize() is invoked.

Correct the comment above the validate_states function to indicate that it does not perform the expiry validation on the order.

In the EVM implementation the MAX_ORACLE_AGE is defined as 1441 minutes, or one day plus one minute. In the Move implementation however this is just 1 day represented in milliseconds.

Consider if there should be parity between the Move and EVM implementations on the MAX_ORACLE_AGE value.

In the calculate_usd_price_internal function there is no sanity validation that ensures that the resulting target_usd_price fits within a u64 value before being cast as such.

It is incredibly unlikely that the value would exceed u64, however in the event of a misconfiguration or oracle malfunction it would be best to include a validation that aborts if the result is unexpectedly found to be larger than the u64 type.

Consider adding a sanity validation that prevents logic from continuing if the target_usd_price result would overflow the u64 cast.

In collateral_manager, transfer_to_custody transfers the redeem balance to the configured custodian_address. However, it does not check whether the collateral is enabled, whereas the Solana equivalent withdraw flow gates withdrawals by checking whether the vault is currently enabled.

Document this as intended behavior, or enforce assert_is_enabled in transfer_to_custody.