Sonata

The reviewed implementation of the stS and wOS price feeds relies on a single price source when _isRedemption == false or when the two sources deviate beyond the set threshold. Specifically:

• stS: Uses only the canonical rate.

• wOS: Uses only the Chainlink oracle, leaving aside the canonical rate.

This approach diverges from Liquity v2’s design for certain LSTs (e.g., RETH), where the minimum of the oracle and canonical rates is used in the no-redemption path to mitigate upward market price manipulation. Such a defensive measure is particularly important for assets with low liquidity, where it may be relatively inexpensive for a motivated attacker to manipulate the market price and influence oracle outcomes.

By relying on a single source, the current implementation increases exposure to such manipulation risks. While Chainlink oracles are resilient to certain manipulation attacks, their reliability ultimately depends on the liquidity depth of the markets they aggregate. If the underlying markets have thin liquidity, attackers may artificially inflate asset prices, resulting in excessive ONE minting, which is detrimental to the entire system health and ONE peg.

In STAStaking, rewards (protocol interest and upfront fees) are distributed pro-rata to the current staker set at the moment interest or fees are minted, rather than based on a time-weighted stake. Because mintAggInterest() can be invoked externally as part of user protocol actions (e.g., opening a trove), a user could deposit immediately before such an action and withdraw immediately afterward to capture a share of the pending yield.

This behavior is particularly exploitable when protocol actions involve substantial upfront fees, as these are not fixed but depend on the system’s average interest rate and the amount of ONE debt drawn. While regular interest minting may not yield significant rewards (due to its periodic and smaller amounts), large upfront fees from high-value actions could be attractive targets.

Example attack scenario:

• An attacker monitors the mempool and detects a large trove-opening transaction, which will generate an upfront fee, part of which will go to STAStaking.
• The attacker frontruns the trove-opening transaction with a deposit into STAStaking.
• The trove-opening transaction executes, minting rewards while the attacker is staked.
• The attacker withdraws shortly afterward, capturing part of the fee despite staking for only a very short period.

Rewards are limited by fee calculation rules and potential user-controlled caps and while it is unclear how consistently such frontrunning could be executed on Sonic, the scenario remains plausible.

Potential mitigations could be (1) locking newly staked positions for a minimum period (e.g., 1 day to 1 week) or (2) Implementing time-weighted reward distribution, such as Synthetix’s StakingRewards model, where rewards are proportional to both stake size and staking duration.

Protocol owners are able to set a configurable ONE debt cap for each branch. Once the cap is reached, any protocol action that mints ONE in that branch is disabled. This mechanism acts as a security measure to prevent certain branches, especially those considered riskier, from growing too large and potentially threatening the health of the overall system.

While this provides additional control and safety, it differs from the original Liquity v2 design, which does not include per-branch debt caps.

WrappedOriginPriceFeed::_fetchPricePrimary performs two calls to _getOracleAnswer(osToSOracle) (via _getCanonicalRate and _getOracleRate) when only one would be sufficient. Although both calls are expected to return the same value, we recommend consolidating them into a single call. This would both reduce gas costs and eliminate the potential, however unlikely, that the two calls might return different results.

STAStaking::_withdraw does not verify that maxBalance > 0 before proceeding with a withdrawal. In the current implementation, _amount is set by the user, and maxBalance represents the user's maximum withdrawable balance. If _amount is initially set to 0, it is assigned the value of maxBalance. However, there is no explicit check that maxBalance is greater than zero before continuing.

function _withdraw(
    address _user, address _receiver, uint256 _amount, bool _isAuthority
) internal {
    bytes32 authorityKey = _getAuthorityKey(msg.sender, _user);

    UserBalance storage userBalanceData = balances[_user];
    uint256 maxBalance = _isAuthority ?
        authorityTracking[authorityKey] : userBalanceData.selfBalance;

    if (_amount == 0) {
        _amount = maxBalance;
    }

    require(maxBalance >= _amount, _isAuthority ?
        "Insufficient authority balance" : "Insufficient balance");

    _claim(_user);
    maxBalance -= _amount;

    // Dedaub: code omitted for brevity
    ...
}

As a result, a withdrawal request could proceed with _amount equal to 0, leading to a meaningless withdrawal operation. While this does not introduce a direct security risk, it may cause confusion for users and unnecessary execution costs.

Both the Withdraw and Deposit events are emitted without including the address of the transaction sender (msg.sender). This results in a loss of information in cases where user != msg.sender, i.e., when a contract or another user interacts with the protocol as an authority on behalf of the actual account holder.

In function STAStaking::_claim, if totalBalanceCached == 0 then every user’s reflective balance is also 0, so there’s no point doing per-user math and transfers.

uint256 last = userYieldSnapshot[_user];
uint256 curr = ShareableMath.rmul(userBalance, yieldPerTokenInRayCached);

if (curr > last) {
    uint256 sendingReward = curr - last;
    REWARD_TOKEN.transfer(_user, sendingReward);
    emit Claimed(_user, sendingReward);
}

This means that the code in the above snippet could be moved inside the if (totalBalanceCached > 0) branch as follows:

if (totalBalanceCached > 0) {
    yieldPerTokenInRayCached += ShareableMath.rdiv(
        currentYieldBalance - yieldBalance, totalBalanceCached
    );

    uint256 last = userYieldSnapshot[_user];
    uint256 curr =
        ShareableMath.rmul(userBalance, yieldPerTokenInRayCached);

    if (curr > last) {
        uint256 sendingReward = curr - last;
        REWARD_TOKEN.transfer(_user, sendingReward);
        emit Claimed(_user, sendingReward);
    }
} else if (currentYieldBalance != 0) {
    // ...
    // Dedaub: code omitted for brevity
}

In the WrappedOriginPriceFeed contract, the constant WOS_TO_S_RATE_ORACLE should be renamed to WOS_TO_OS_RATE_ORACLE, and the storage variable wosToSOracle should be renamed to wosToOSOracle, so that their names accurately reflect the actual price feed asset pair.

In the STOracle::_fetchPricePrimary function, the tuple assignment

(uint256 stsPerS, bool exchangeRateIsDown) = _getCanonicalRate();

should use sPerSTS instead of stsPerS to more accurately reflect the meaning of the value (S tokens per STS token).