6.1 The _totalAssets cannot be written down to reflect realized losses
The StratPerpetualBond vault is designed as an ERC-4626 vault where deposited assets are immediately transferred to a manager. This manager is responsible for deploying the assets into a yield-generating strategy, such as selling ETH call options. The vault itself does not hold the underlying assets; its value is tracked by the _totalAssets state variable, which is increased when the manager directs yield back into the vault via the increaseAssetsPerShare(...) function. By design, withdrawals are disabled, and the primary exit path for users is to sell their PBS (vault share) tokens on a secondary market like a DEX.
The potential issue arises if the project decides to enable withdrawals, for instance, to sunset the vault. The _withdraw(...) function calls super._withdraw(...), which in turn attempts to transfer the underlying asset from the vault contract to the user. This transaction will revert if the vault's actual balance of the asset is insufficient to cover the withdrawal amount. Since the vault sends all assets to the manager upon deposit, its balance is typically zero.
function _withdraw(
address caller,
address receiver,
address owner,
uint256 assets,
uint256 shares
) internal virtual override nonReentrant {
// @audit Withdrawals are disabled by default.
if (withdrawalsDisabled) revert WithdrawalsDisabled();
// @audit-issue This call will attempt an asset transfer and revert if the contract has an insufficient balance.
super._withdraw(caller, receiver, owner, assets, shares);
// @audit The total assets are reduced only after a successful withdrawal.
_totalAssets -= assets;
}
If the manager becomes insolvent or its strategy underperforms, it may be unable to return enough assets to the vault to cover all outstanding _totalAssets. In this scenario, enabling withdrawals would create a race condition where the realized losses are not shared proportionally among all shareholders. The first users to withdraw will successfully redeem their shares for assets at a value calculated from the artificially high _totalAssets. These initial withdrawals will deplete the limited asset balance held within the contract. Once this balance is exhausted, subsequent withdrawal attempts will revert, leaving the remaining shareholders unable to recover any funds, as the contract lacks a mechanism to write down the value of _totalAssets to account for the manager's losses.
Recommendation
This design places a high degree of trust in the manager. It is recommended to clearly document these risks for all users, emphasizing that the value of PBS tokens and the viability of an exit are entirely dependent on the manager's performance and solvency.
Adding a function to write down the _totalAssets value would allow for the fair socialization of losses, preventing a "race to the exit." However, such a function introduces a significant centralization risk. If the owner's private key were compromised, an attacker could call this function to maliciously reduce or erase the value of all shares, which would have a severe negative impact on the PBS token's economics.
This finding serves an informational purpose for all stakeholders to understand the design trade-offs. The decision of whether to introduce a loss-socialization mechanism at the cost of increased centralization risk is left to the team.
Resolution
Noted, on balance will keep as is, if we need to socialise losses, we can always do so by creating a new proportional withdraw in a different smart contract as opposed to using withdraw directly
Withdraw is left for completeness to the standard, and to ensure flexibility in the event we want to sunset and return capital easily without using the intended method of users selling shares in the LP pool.
6.2 Vault design does not account for fee-on-transfer tokens
The vault's accounting mechanism in the _deposit(...) function is incompatible with fee-on-transfer tokens. It credits the vault's _totalAssets based on the user-specified deposit amount before the actual token transfer occurs. If the vault were deployed with a fee-on-transfer stablecoin, the amount received by the manager would be less than the amount recorded, causing the vault's total asset value to become inflated over time. This would lead to shares being backed by fewer assets than the accounting suggests. While uncommon, some stablecoins like USDT have a dormant fee mechanism, which presents a consideration for future deployments.
Recommendation
When deploying new instances of the vault with different stablecoins, care must be taken to ensure the chosen asset does not have a fee-on-transfer mechanism to avoid accounting inaccuracies.
Resolution
We'll be using USDS (sky.money) - given the probability sky/maker will add a fee is low, we think this is acceptable risk.
6.3 Incorrect comment for depositCap
The StratPerpetualBond contract uses a depositCap state variable to enforce a limit on the total assets that can be deposited. The NatSpec comment associated with this variable states that a value of 0 signifies no deposit cap.
// ...
/// @dev Maximum total assets that can be deposited (0 = no cap)
// @audit-issue This comment is incorrect. A value of 0 effectively blocks all deposits.
uint256 public depositCap;
// ...
However, the implementation within the _deposit(...) function contradicts this comment. The function checks if the new total assets would exceed the cap.
function _deposit(
address caller,
address receiver,
uint256 assets,
uint256 shares
) internal virtual override {
// @audit If depositCap is 0, this check will revert for any deposit with assets > 0.
if (_totalAssets + assets > depositCap) {
revert DepositCapExceeded();
}
super._deposit(caller, receiver, assets, shares);
// ...
}
If depositCap is set to 0, the condition _totalAssets + assets > depositCap will be true for any deposit where assets is greater than 0. This will cause the transaction to revert, effectively disabling all deposits.
This discrepancy between the comment and the code's behavior can mislead the contract owner or any developer interacting with the system. An owner attempting to remove the deposit limit by setting depositCap to 0 would unintentionally halt all deposits to the vault.
Recommendation
Consider correcting the comment for depositCap to accurately reflect its functionality.
Resolution
Resolved in commit dad2bf03a36d41076d211c02954d67f5c426827d.
6.4 The increaseAssetsPerShare(...) function could emit an event
The StratPerpetualBond contract allows a manager to add yield to the vault by calling the increaseAssetsPerShare(...) function. This function increases the total assets under management (_totalAssets), effectively boosting the value of shares held by depositors.
However, the increaseAssetsPerShare(...) function does not emit an event after successfully adding assets.
function increaseAssetsPerShare(uint256 assets) external {
// @audit-issue No event is emitted to signal the addition of assets.
SafeERC20.safeTransferFrom(IERC20(asset()), msg.sender, address(this), assets);
SafeERC20.safeTransfer(IERC20(asset()), manager, assets);
_totalAssets += assets;
}
The absence of an event makes it difficult for off-chain services, monitoring tools, and users to track the inflow of yield into the vault. This lack of transparency complicates the analysis of the vault's performance over time.
Recommendation
Consider defining and emitting an event within the increaseAssetsPerShare(...) function.
Resolution
Resolved in commit 6ccf6ff559e7a23f3c71f673ffaf273e7ca1567f.
