Hi — I do smart contract security reviews and ran a quick pass over BasketVault.sol. Found one critical bug that breaks the partial-redemption path entirely, plus a few medium-severity issues. Happy to do a full review for free if you are heading toward mainnet.
Critical — redeem() partial-fill path always reverts
File: src/vault/BasketVault.sol ~L270
When local reserves are insufficient the code calls:
shareToken.transferFrom(msg.sender, address(this), remainderShares);
BasketShareToken is a plain ERC-20. This transferFrom checks the caller's allowance. The vault is never pre-approved, so this will revert for every user who has not separately called shareToken.approve(vault, amount) before redeeming — which no normal user would do.
Fix: replace with shareToken.burn(msg.sender, remainderShares) (vault-only, no allowance needed) and record the USDC debt in the PendingRedemption struct. The vault already does shareToken.burn(address(this), pr.sharesLocked) in processPendingRedemption, which would also need updating.
Medium — stale global PnL silently zeroed on spokes
_pricingNav() ignores globalAdj when the relay reports stale data:
(int256 pnl, bool stale) = stateRelay.getGlobalPnLAdjustment(address(this));
if (!stale) globalAdj = pnl;
On spoke chains vaultAccounting is address(0), so localPnL = 0. The entire PnL component of NAV is dropped on staleness. A griefer who can stall relay updates could force NAV to equal raw idle-USDC, allowing cheap share purchases before the relay catches up.
Fix: revert deposits/redeems when the relay is stale, or surface staleness to the caller.
Medium — pending redemptions have no expiry or user-cancel path
Once RedemptionQueued is emitted the user's shares are locked in the vault forever unless the keeper calls processPendingRedemption. There is no timeout, no keeper liveness check, and no way for the user to reclaim their shares if the keeper goes offline.
Fix: add a cancelPendingRedemption(uint256 id) that returns shares to the user after a deadline (e.g. 72 h).
Low — perpAllocated can diverge from actual exposure
withdrawFromPerp silently clamps:
if (amount >= perpAllocated) { perpAllocated = 0; }If more USDC is withdrawn than was booked (e.g. due to realised gains credited externally) the book entry stays at 0 while actual perp exposure is non-zero. NAV will under-count until the next manual reconciliation.
I can provide a full report (all contracts, including the GMX-fork perp stack and cross-chain relay) if useful. No cost — would just like to contribute before a mainnet launch. Let me know if you are interested.
Hi — I do smart contract security reviews and ran a quick pass over BasketVault.sol. Found one critical bug that breaks the partial-redemption path entirely, plus a few medium-severity issues. Happy to do a full review for free if you are heading toward mainnet.
Critical — redeem() partial-fill path always reverts
File: src/vault/BasketVault.sol ~L270
When local reserves are insufficient the code calls:
BasketShareToken is a plain ERC-20. This transferFrom checks the caller's allowance. The vault is never pre-approved, so this will revert for every user who has not separately called shareToken.approve(vault, amount) before redeeming — which no normal user would do.
Fix: replace with shareToken.burn(msg.sender, remainderShares) (vault-only, no allowance needed) and record the USDC debt in the PendingRedemption struct. The vault already does shareToken.burn(address(this), pr.sharesLocked) in processPendingRedemption, which would also need updating.
Medium — stale global PnL silently zeroed on spokes
_pricingNav() ignores globalAdj when the relay reports stale data:
On spoke chains vaultAccounting is address(0), so localPnL = 0. The entire PnL component of NAV is dropped on staleness. A griefer who can stall relay updates could force NAV to equal raw idle-USDC, allowing cheap share purchases before the relay catches up.
Fix: revert deposits/redeems when the relay is stale, or surface staleness to the caller.
Medium — pending redemptions have no expiry or user-cancel path
Once RedemptionQueued is emitted the user's shares are locked in the vault forever unless the keeper calls processPendingRedemption. There is no timeout, no keeper liveness check, and no way for the user to reclaim their shares if the keeper goes offline.
Fix: add a cancelPendingRedemption(uint256 id) that returns shares to the user after a deadline (e.g. 72 h).
Low — perpAllocated can diverge from actual exposure
withdrawFromPerp silently clamps:
If more USDC is withdrawn than was booked (e.g. due to realised gains credited externally) the book entry stays at 0 while actual perp exposure is non-zero. NAV will under-count until the next manual reconciliation.
I can provide a full report (all contracts, including the GMX-fork perp stack and cross-chain relay) if useful. No cost — would just like to contribute before a mainnet launch. Let me know if you are interested.