Obsidian OBSDN Perpetual Protocol Security Review Report

The AccountModule allows an account to delegate signing rights by registering a signer in registerSigner. This is permissionless and requires signatures from both the account and the delegated signer.

It first validates the signature of the account:

bytes32 registerHash = obsdn.hashTypedDataV4(
    keccak256(abi.encode(REGISTER_TYPEHASH, signer, keccak256(abi.encodePacked(params.message)), nonce))
);
 
if (!sigValidator.isValidSig(sender, registerHash, params.senderSignature)) {
    revert Errors.InvalidSig(sender);
}

The function isValidSig will first attempt ERC-1271 signature verification if the sender address has code. This calls the static function isValidSignature(hash, signature) to the address and expects a magic value that represents success.

This ERC-1271 signature verification enables the usage of smart wallets, where a contract acts as account/signer and can be owned by one or multiple owners. For example, the popular Argent smart wallet simply performs it's own ECDSA signature verification on the hash and signature and checks if it equals the wallets owner: argent-contracts/contracts/modules/TransactionManager.sol at 0e9bf648ae65e6c5e1d44e0e7cbaa46ff813d878 · argentlabs/argent-contracts

Now the Register hash comes from (REGISTER_TYPEHASH, signer, message, nonce). The signer parameter is the receiving delegated signer, so the EOA / smart wallet that is verifying the signature here is not included in the digest hash.

The issue here is that if a smart wallet's owner has 2 or more Argent smart wallet accounts on OBSDN, then the same signature would be valid for both accounts. This is because both accounts will delegate signature verification to isValidSignature, which simply checks that the wallet owner has signed the hash. The hash would be the same, but the final state change is different:

signerWallets[sender][signer] = true;

Because sender is not part of the signed data and only performs the signature verification. Which as shown above, can be spoofed for ERC1271 smart wallets.

Consider the scenario:

1. Owner A has a smart wallet X and smart wallet Y on OBSDN.

2. Owner A signs a Register(P, msg, nonce) hash to allow address P to sign for smart wallet X, P also signs DelegatedSigner(X).

3. Owner A calls registerSigner with both signatures to make address P be a signer for account X.

4. Address P can now sign DelegatedSigner(Y) themselves and take the same signature for Register(P, msg, nonce) to submit this to the sequencer and call processRegisterSigner for account Y and register themselves for the account Y, escalating their privileges to another vault.

This is privilege escalation, since owner A only intended for account X to be delegated to address P. If account X was a small vault designed for untrusted users, while account Y is a large treasury, then the result could be direct theft of assets.

The directly callable registerSigner is guarded by a msg.sender check, but the processRegisterSigner function only relies on the signature. The nonce check would be applied to the new params.sender, so it would be likely to pass. The processRegisterSigner function is only callable by the sequencer so assume that the attacker can submit this request and signature to an API where the sequencer will forward the request on-chain.

function registerSigner(
    IObsdn obsdn,
    mapping(address account => mapping(address signer => bool isRegistered)) storage signerWallets,
    IObsdn.RegisterSignerParams memory params
)
    external
{
    ISigValidator sigValidator = obsdn.getSigValidator();
 
    address sender = params.sender;
    address signer = params.signer;
    uint64 nonce = params.nonce;
 
    bytes32 registerHash = obsdn.hashTypedDataV4(
        keccak256(abi.encode(REGISTER_TYPEHASH, signer, keccak256(abi.encodePacked(params.message)), nonce))
    );
    if (!sigValidator.isValidSig(sender, registerHash, params.senderSignature)) {
        revert Errors.InvalidSig(sender);
    }
 
    bytes32 signKeyHash = obsdn.hashTypedDataV4(keccak256(abi.encode(DELEGATED_SIGNER_TYPEHASH, sender)));
    if (!sigValidator.isValidSig(signer, signKeyHash, params.signerSignature)) {
        revert Errors.InvalidSig(signer);
    }
 
    signerWallets[sender][signer] = true;
 
    emit IObsdn.RegisterSigner(sender, signer, nonce);
}

The sender parameter should also become part of the registration hash digest:

Register(address sender,address signer,string message,uint64 nonce)

This ensures that one cannot swap out sender for another account when calling registerSigner.

The protocol uses certain typescript scripts in order to deploy the protocol. One of the main scripts is the scripts/deploy/DeployObsdn.ts which is responsible for deploying all of the main parts of the protocol. The script first deploys the contracts then in separate transactions initializes them. This creates an opportunity for a potential attacker to front-run the init transactions and initialize the contracts themselves. The script does have a single check that tries to verify whether the contract is already initialized or no:

const isInitialized = async (contractName: string, contractAddress: string): Promise<boolean> => {
    const contract = await ethers.getContractAt(contractName, contractAddress);
    const obsdnAddr = await contract.obsdn();
    return obsdnAddr !== ethers.ZeroAddress;
};

The check is being used in the following way:

console.log("\n8. Initializing SpotLedger...");
await ensureInitialized(
    state,
    "SpotLedger",
    () => isInitialized("SpotLedger", spotLedgerAddress),
    async () => {
        const spotLedger = await ethers.getContractAt("SpotLedger", spotLedgerAddress);
        const tx = await spotLedger.initialize(obsdnAddress, matchingAddress, vaultManagerAddress);
        await tx.wait();
    },
);

The issue is that even if the contract is initialized the script doesn't halt or give any notable warning besides just stating the the contract was already initialized:

export async function ensureInitialized(
    state: DeployState,
    name: string,
    checkFn: () => Promise<boolean>,
    initFn: () => Promise<void>,
): Promise<void> {
    // Check on-chain first (source of truth)
    const alreadyDone = await checkFn();
    if (alreadyDone) {
        if (!state._deployState.initialized[name]) {
            state._deployState.initialized[name] = true;
            saveDeployState(state);
        }
        console.log(` ${name} already initialized — skipping.`);
        return;
    }
 
    await initFn();
    state._deployState.initialized[name] = true;
    saveDeployState(state);
    console.log(` ${name} initialized.`);
}

This can lead to a case where a maliciously initialized contract stays in production. The second issue with the isInitialized check is that even if the script reverted the check can simply be bypassed by simply setting the obsdn address to 0 as it is the only thing being checked on the deployed contracts and thus creating a DoS vector as even if the tx of the deployment script fails the script doesn't check the return result and thus continues the deployment as is and because the obsdn was initialized with the 0 address any calls to the protocol will mostly fail.

Create an atomic deployment and init script that utilizes CREATE2.

Vault staking and unstaking price shares only from the vault account's spot balance in SpotLedger. However, vault accounts can also hold open perpetual positions in PerpLedger. Opening or increasing a perp position updates only position.size and position.quoteBalance, it does not settle any quote amount into spot balance until the position is reduced, flipped, or closed (another trade is applied via matchOrders function).

function getTotalAssets(address vault) public view returns (int256) {
    return spotLedger.getBalance(vault, stakingToken);
}
 
function convertToShares(address vault, uint256 assets) public view returns (uint256) {
    int256 totalAssets = getTotalAssets(vault);
    uint256 totalShares = getTotalShares(vault);
 
    uint256 effectiveTotalAssets = totalAssets < 0 ? 0 : uint256(totalAssets);
    return assets.mulDiv(totalShares + 1, effectiveTotalAssets + 1);
}

As a result, a vault with material unrealized perp PnL can still appear solvent and allow stakers to stake or unstake before the PnL are realized. It allows malicious user to unstake at original NAV for exiting losses of perp position or stake at underpriced NAV to claim profit of vault's perp position.

Scenario:

1. A vault has 1,000 USDC spot balance and 1,000 shares. Alice owns 500 shares and Bob own the remaining 500.
2. The vault opens a perp position. Its spot balance remains 1,000 USDC because opening/increasing a position settles 0 to SpotLedger.
3. The market moves against the vault, creating 500 USDC of unrealized loss. But no trade is applied to realized that loss yet.
4. Alice submits a valid signed unstake for 500 USDC. unstake() burns approximately 500 shares and transfers 500 USDC out based on stale spot NAV (vault spot balance = 500).
5. The vault later closes the losing perp position by matchOrders operation. The 500 USDC loss is then settled into the vault spot balance, leaving Bob with no assets left (vault spot balance = 0). Similarly, a new staker in the vault can stake before profits from perpetual positions are realized in the vault, then claim a part of those profits.

Condition:

Currently, the availability of stake/unstake actions on unrealized perpetual PnL depends on the sequencer's discretionary ordering of operations. However, there are no documented rules or restrictions regarding the order of stake/unstake operations in BACKEND_INTEGRATION_GUIDE.md, UPGRADE_GUIDE.md, or SEC_PROMPT.md. Tests in Integration.t.sol follow an open → close → unstake pattern (e.g. test 3, 4, 5), but do not assert that stake/unstake-during-open reverts.

Therefore, the on-chain vault accounting should either include open perp equity in the vault NAV or block stake/unstake while vault perp PnL is unsettled.

If vaults are intended to support stake/unstake with open positions, add a trusted on-chain mark-to-market equity calculation and use that value in getTotalAssets(). Otherwise, vault should block staking and unstaking when it has open perp positions.

When a user stakes into a vault with a negative spot balance, the deficit is first repaid from the staker's balance. Only the remaining amount is then used to mint shares and record the staker's avgPrice.

uint256 effectiveAmountX18 = _settleVaultDeficit(vault, staker, amountX18);
shares = convertToShares(vault, effectiveAmountX18);
if (shares == 0) {
    revert Errors.VaultManager_ZeroShares();
}
_processStake(vault, staker, effectiveAmountX18, shares);
spotLedger.stakeVault(staker, vault, stakingToken, effectiveAmountX18);

The deficit repayment is an actual debit from the staker.

uint256 deficitAmountX18 = uint256(-totalAssets);
if (amountX18 < deficitAmountX18) {
    revert Errors.VaultManager_InsufficientAmountToSettleDeficit(vault, staker, deficitAmountX18, amountX18);
}
 
spotLedger.settleVaultDeficit(staker, vault, stakingToken, deficitAmountX18);
effectiveAmountX18 = amountX18 - deficitAmountX18;

However, _processStake records the cost basis using only the post-deficit residual amount.

uint256 prevShares = position.shares;
uint256 currentPrice = amountX18.mulDiv(Math.X18, shares);
 
if (prevShares == 0) {
    position.avgPrice = currentPrice;
} else {
    position.avgPrice = (prevShares * position.avgPrice + shares * currentPrice) / (prevShares + shares);
}

Later, unstake fees are calculated against this recorded avgPrice.

uint256 currentPrice = amountX18.mulDiv(Math.X18, shares);
if (currentPrice > position.avgPrice) {
    uint256 profit = shares.mulDiv(currentPrice - position.avgPrice, Math.X18);
    feeX18 = profit.mulDiv(vaultData.profitShareBps, MAX_BPS);
}

As a result, a deficit-paying staker can have a lower recorded basis than the amount actually debited from them. If the vault later recovers, profit-share fees can be charged before the staker has recovered their full gross contribution.

Include deficit repayments made by the staker in the profit-share cost basis.

Keep share minting based on the post-deficit residual amount, but track fee basis separately if needed.

VaultManager.stake routes the staker's input amount through _settleVaultDeficit before computing shares. When the vault holds a deficit, the deficit portion is transferred from the staker to the vault, and shares are then minted against the residual effectiveAmountX18 using convertToShares. By that point, totalAssets has already been brought back to zero.

uint256 effectiveAmountX18 = _settleVaultDeficit(vault, staker, amountX18);
shares = convertToShares(vault, effectiveAmountX18);
if (shares == 0) {
    revert Errors.VaultManager_ZeroShares();
}
_processStake(vault, staker, effectiveAmountX18, shares);
spotLedger.stakeVault(staker, vault, stakingToken, effectiveAmountX18);

function convertToShares(address vault, uint256 assets) public view returns (uint256) {
    int256 totalAssets = getTotalAssets(vault);
    uint256 totalShares = getTotalShares(vault);
 
    uint256 effectiveTotalAssets = totalAssets < 0 ? 0 : uint256(totalAssets);
    return assets.mulDiv(totalShares + 1, effectiveTotalAssets + 1);
}

int256 totalAssets = getTotalAssets(vault);
if (totalAssets >= 0) {
    return amountX18;
}
 
uint256 deficitAmountX18 = uint256(-totalAssets);
if (amountX18 < deficitAmountX18) {
    revert Errors.VaultManager_InsufficientAmountToSettleDeficit(
        vault, staker, deficitAmountX18, amountX18
    );
}
 
spotLedger.settleVaultDeficit(staker, vault, stakingToken, deficitAmountX18);
effectiveAmountX18 = amountX18 - deficitAmountX18;

This produces a sharp discontinuity around the deficit amount. A stake below the deficit reverts with InsufficientAmountToSettleDeficit. A stake equal to the deficit reverts with ZeroShares because effectiveAmountX18 becomes zero. A stake of deficit + 1 wei, however, succeeds: with effectiveAmountX18 = 1 and effectiveTotalAssets = 0, the share calculation reduces to 1 * (totalShares + 1) / 1, issuing roughly the entire pre-existing share supply for a 1 wei residual contribution. The deficit portion of the stake is used to restore the vault balance to zero before shares are computed, while only the residual amount is used as the asset input for share issuance.

If the vault later accrues value through accounting updates that increase totalAssets without minting proportional shares, the shares minted at this boundary entitle the staker to a fraction of that value that is not commensurate with the 1 wei residual contribution, and existing stakers' claim on that value is diluted accordingly.

Disallow normal staking while a vault with non-zero totalShares holds non-positive totalAssets.

Separate deficit repayment from share issuance so that covering a deficit and minting shares are distinct operations rather than two effects of a single call.

If staking is intended to recapitalize an underwater vault, define an explicit share-issuance rule for that path instead of computing shares against the post-settlement zero-asset state.

VaultManager.unstake accepts an asset-denominated amount, converts it to shares, and burns the resulting share amount. The conversion uses mulDiv without an explicit rounding mode, so the result is rounded down. The vault is then debited by the full requested asset amount in SpotLedger.unstakeVault.

amountUnstakedX18 = amountX18;
shares = convertToShares(vault, amountUnstakedX18);
if (shares == 0) {
    revert Errors.VaultManager_ZeroShares();
}
feeX18 = _processUnstake(vault, staker, amountUnstakedX18, shares);
feeRecipient = vaults[vault].operator;
 
spotLedger.unstakeVault(vault, staker, stakingToken, amountUnstakedX18, feeRecipient, feeX18);

For an asset-denominated unstake, rounding the share burn down can leave the staker burning slightly fewer shares than required for the requested asset amount. This creates a small accounting mismatch in favor of the exiting staker.

function convertToShares(address vault, uint256 assets) public view returns (uint256) {
    int256 totalAssets = getTotalAssets(vault);
    uint256 totalShares = getTotalShares(vault);
    uint256 effectiveTotalAssets = totalAssets < 0 ? 0 : uint256(totalAssets);
    return assets.mulDiv(totalShares + 1, effectiveTotalAssets + 1);
}

Round the share amount up when converting an asset-denominated unstake amount into shares to burn.

Keep the stake minting path and unstake burn path separate if they require different rounding directions.

The integration doc declares the order typestring (L72):

Order(address sender,uint8 productIndex,uint8 side,uint128 size,uint128 price,uint64 nonce)

The contract:

// OrderModule.sol L12–13
bytes32 public constant ORDER_TYPEHASH =
    keccak256("Order(address sender,uint8 marketIndex,uint8 side,uint128 size,uint128 price,uint64 nonce)");

The field name productIndex in the doc and marketIndex in the contract are mismatching. EIP-712 type hashes include field names, so signatures produced with productIndex will not verify against the contract's marketIndex type hash. Those orders would revert with InvalidSig.

Similarly, the type string of transfer also has mismatched field names between integration docs and contract code.

The back-end integration use sender name, but the contract use from.

Transfer(address sender,address to,address token,uint128 amount,uint64 nonce)

bytes32 public constant TRANSFER_TYPEHASH =
    keccak256("Transfer(address from,address to,address token,uint128 amount,uint64 nonce)");

Update the docs to use the correct field names of EIP-712 type hash for orders and transfers.