Skip to content

Feat/card borrow mode - #1285

Merged
MayankMittal1 merged 3 commits into
qafrom
feat/card_borrow_mode
Jan 19, 2026
Merged

Feat/card borrow mode#1285
MayankMittal1 merged 3 commits into
qafrom
feat/card_borrow_mode

Conversation

@MayankMittal1

Copy link
Copy Markdown
Contributor

No description provided.

@vercel

vercel Bot commented Jan 18, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Review Updated (UTC)
solid-app Ignored Ignored Jan 18, 2026 8:17pm
solid-app-staging Ignored Ignored Jan 18, 2026 8:17pm

Request Review

@claude

claude Bot commented Jan 18, 2026

Copy link
Copy Markdown

Code Review: PR #1285 - Card Borrow Mode Feature

Thank you for this substantial feature addition! This PR implements borrowing against soUSD collateral via Aave V3. While the implementation shows good architectural patterns, there are several critical security and financial safety issues that must be addressed before merging.

🔴 Critical Issues (Must Fix)

1. LTV Ratio Mismatch and Hardcoding

File: hooks/useBorrowAndDepositToCard.ts:47

const soUSDLTV = 79n; // 80% LTV for soUSD (79% to avoid rounding errors)

Issues:

  • Comment says 80% but value is 79% - which is correct?
  • Hardcoded value could become outdated if Aave governance changes LTV
  • No validation against actual Aave protocol parameters

Recommendation: Fetch LTV from Aave's pool configuration contract instead of hardcoding. If hardcoding is necessary for performance, add comprehensive documentation and consider periodic validation.


2. Dangerous Default Slippage (5%)

File: hooks/useBorrowAndDepositToCard.ts:160, 199

const dstAmountMin = (borrowAmountWei * 95n) / 100n; // 5% slippage

Issues:

  • 5% slippage is extremely high and could cost users significant value
  • For a $1000 borrow, this allows up to $50 loss
  • No user control or warnings

Recommendation:

  • Reduce default to 0.5-1% maximum
  • Make slippage user-configurable
  • Add warnings for high-value transactions
  • Consider dynamic slippage based on liquidity depth

3. Race Condition in Collateral Calculation

File: hooks/useRepayAndWithdrawCollateral.ts:80-109

const rate = await readContract(publicClient(mainnet.id), {
  address: ADDRESSES.ethereum.accountant,
  abi: ACCOUNTANT_ABI,
  functionName: 'getRate',
});
// ... calculations ...
// ... later transaction execution

Issues:

  • soUSD exchange rate is fetched off-chain
  • Rate could change between fetch and transaction execution
  • Could lead to under-collateralization or transaction failures

Recommendation:

  • Fetch rate on-chain within the transaction itself
  • Add staleness checks with maximum age tolerance
  • Implement rate bounds checking

4. Missing Health Factor Validation

File: hooks/useRepayAndWithdrawCollateral.ts:92-109

const requiredCollateralValueWei =
  remainingBorrowWei === 0n
    ? 0n
    : (remainingBorrowWei * TARGET_HEALTH_FACTOR_BPS + (LIQ_THRESHOLD_BPS - 1n)) /
      LIQ_THRESHOLD_BPS;

Issues:

  • TARGET_HEALTH_FACTOR_BPS = 10_200n (1.02x) is dangerously low
  • Aave liquidates at 1.0, leaving only 2% buffer
  • No validation that withdrawal keeps health factor safe
  • No protection against price volatility

Recommendation:

  • Increase minimum health factor to 1.25x (preferably 1.5x)
  • Add pre-transaction health factor validation
  • Block withdrawals that would drop below safe threshold
  • Add user warnings when health factor < 1.5

5. No Health Factor Display

Missing: UI doesn't show health factor anywhere

Issues:

  • Users are blind to liquidation risk
  • No way to monitor position health
  • Critical for DeFi lending safety

Recommendation:

  • Add health factor display to BorrowPositionCard
  • Color-code: green (>1.5), yellow (1.1-1.5), red (<1.1)
  • Show estimated liquidation price
  • Add push notifications for health factor < 1.3

6. Uncapped Borrow Amount

File: components/Card/CardDepositInternalForm.tsx:533-538

const maxBorrowAmount = useMemo(() => {
  if (soUsdBalanceAmount > 0 && exchangeRate > 0) {
    return soUsdBalanceAmount * exchangeRate * 0.8; // 70% of savings value
  }
  return 0;
}, [soUsdBalanceAmount, exchangeRate]);

Issues:

  • Comment says "70%" but calculation is * 0.8 (80%)
  • No check against Aave's available USDC liquidity
  • Could exceed Aave's borrow cap
  • Large borrows could fail after collateral is supplied

Recommendation:

  • Fix comment/code mismatch
  • Fetch available borrow liquidity from Aave
  • Cap to min(calculated_max, available_liquidity * 0.95)

🟡 High Priority Issues

7. Missing Bridge Failure Handling

File: hooks/useBorrowAndDepositToCard.ts:262-286

Issues:

  • No handling for LayerZero bridge failures
  • No retry mechanism
  • Funds could be stuck if bridge fails
  • No way to recover from partial execution

Recommendation:

  • Add bridge transaction status polling
  • Implement retry logic with exponential backoff
  • Provide refund mechanism for failed bridges
  • Add timeout and manual recovery options

8. Transaction Atomicity Risk

File: hooks/useBorrowAndDepositToCard.ts:217-258

const transactions = [
  { to: ADDRESSES.fuse.vault, data: supplyApproveCalldata },
  { to: ADDRESSES.fuse.aaveV3Pool, data: supplyCalldata },
  { to: ADDRESSES.fuse.aaveV3Pool, data: borrowCalldata },
  { to: USDC_STARGATE, data: approveUSDCCalldata },
  { to: ADDRESSES.fuse.bridgePaymasterAddress, ... },
];

Issues:

  • Multi-step transaction without rollback mechanism
  • If step 3+ fails, approvals remain active (security risk)
  • Could leave user in inconsistent state
  • No cleanup on partial failure

Recommendation:

  • Add comprehensive error handling for each step
  • Reset approvals on failure
  • Implement state cleanup/recovery mechanism
  • Consider batching critical steps atomically

9. No Gas Estimation

File: All transaction hooks

Issues:

  • Complex multi-step transactions could run out of gas
  • No pre-flight gas check
  • Users might fail mid-transaction

Recommendation:

  • Add gas estimation before transaction execution
  • Display estimated gas costs in UI
  • Check sufficient balance for gas + amount
  • Add buffer for gas price fluctuation

10. Missing Input Validation

File: components/Card/CardRepayForm.tsx:108-121

Issues:

  • No maximum value check (could overflow JavaScript numbers)
  • No decimal precision validation (USDC is 6 decimals)
  • No minimum amount validation (dust amounts could fail)

Recommendation:

.refine(val => Number(val) < Number.MAX_SAFE_INTEGER, {
  error: 'Amount too large'
})
.refine(val => {
  const decimals = val.split('.')[1]?.length || 0;
  return decimals <= 6;
}, {
  error: 'USDC supports maximum 6 decimal places'
})
.refine(val => Number(val) >= 0.01, {
  error: 'Minimum repay amount is 0.01 USDC'
})

🟠 Medium Priority Issues

11. Performance: Excessive Re-renders

File: components/Card/CardDepositInternalForm.tsx:757-804

Issues:

  • Two useEffect hooks with overlapping dependencies
  • Slider updates trigger form updates which trigger slider updates
  • Could cause infinite loops or jank

Recommendation:

  • Combine effects or use refs to prevent cascading updates
  • Debounce slider value changes (100-300ms)
  • Use useCallback to stabilize functions

12. Inconsistent Error Handling

File: components/Card/CardRepayForm.tsx:152-210

} catch (error) {
  setRepayStatus(Status.ERROR);
  Toast.show({
    type: 'error',
    text1: 'Repay failed',
    text2: 'Please try again or check your wallet balance',
  });
}

Issues:

  • Generic error message doesn't help users
  • No error type differentiation
  • No suggested remediation steps

Recommendation:

  • Parse error types (insufficient balance, insufficient gas, network error, etc.)
  • Provide specific actionable messages
  • Add error tracking with context

13. TypeScript: Any Types

File: components/Card/CardDepositInternalForm.tsx:225, 580, 652

const onSubmit = async (data: any) => {

Issue: Using any defeats TypeScript's type safety

Recommendation:

const onSubmit = async (data: FormData) => {

14. Accessibility Missing

File: Multiple components

Issues:

  • No accessibilityRole on Pressable components
  • No accessibilityLabel or accessibilityHint
  • Screen readers won't work properly

Recommendation:

<Pressable
  onPress={handlePress}
  accessibilityRole="button"
  accessibilityLabel="Repay borrowed amount"
  accessibilityHint="Opens modal to repay your borrowed USDC"
>

15. Magic Numbers and Documentation

File: components/Card/BorrowSlider.tsx:47

const soUSDLTV = 79n;

Issues:

  • No explanation why 79% not 80%
  • Step size comment says "0.1" but code is 0.01
  • Slider positioning calculation lacks comments

Recommendation: Add comprehensive inline documentation explaining financial calculations


✅ Positive Observations

  1. Good separation of concerns - Business logic properly extracted to hooks
  2. Consistent patterns - Follows existing codebase conventions well
  3. Error tracking - Good use of Sentry for monitoring
  4. Type safety - Generally good TypeScript usage with proper viem types
  5. State management - Clean Zustand store implementation
  6. User feedback - Good loading states and toast notifications

📋 Testing Requirements

Before merging, please add:

  1. Unit tests:

    • Collateral calculation logic
    • Health factor calculations
    • Edge cases (0 amounts, max amounts, etc.)
  2. Integration tests:

    • Aave interaction flows
    • Bridge transaction flows
    • Error recovery paths
  3. E2E tests:

    • Complete borrow → deposit → repay flow
    • Liquidation scenario handling
    • Multi-step transaction failures
  4. Fuzz testing:

    • Random input values
    • Extreme market conditions
    • Rate volatility scenarios

📚 Documentation Needs

Please add:

  1. Inline comments explaining:

    • Why LTV is 79% vs 80%
    • Health factor target rationale
    • Slippage tolerance reasoning
  2. User-facing docs:

    • Liquidation risk explanation
    • How health factor works
    • When to repay vs withdraw
  3. Developer docs:

    • Architecture decision records
    • Integration with Aave V3
    • Bridge failure recovery process

Summary

This is a well-structured feature implementation, but it handles real user funds in a DeFi lending protocol. The critical issues above must be addressed before merging to prevent:

  • User liquidations due to insufficient warnings
  • Fund loss from excessive slippage
  • Stuck transactions from race conditions
  • Financial loss from calculation errors

Please prioritize the 🔴 Critical Issues, as they present real financial risks to users. The 🟡 High Priority issues should also be addressed to ensure robust production operation.

Happy to discuss any of these points further. Great work on the overall architecture!

@MayankMittal1
MayankMittal1 merged commit 4c3412d into qa Jan 19, 2026
5 checks passed
@LiorAgnin
LiorAgnin deleted the feat/card_borrow_mode branch January 22, 2026 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant