Skip to content

Comments

fix: implement repayAndClosePosition to enable collateral withdrawal#16

Closed
kgrgpg wants to merge 2 commits intocadence-testing-patterns-forkfrom
fix-collateral-withdrawal
Closed

fix: implement repayAndClosePosition to enable collateral withdrawal#16
kgrgpg wants to merge 2 commits intocadence-testing-patterns-forkfrom
fix-collateral-withdrawal

Conversation

@kgrgpg
Copy link
Contributor

@kgrgpg kgrgpg commented Jun 19, 2025

Summary

This PR fixes the critical issue discovered in #14 where users cannot retrieve their collateral after repaying their loans.

Problem

  • Users could repay MOET debt but collateral remained locked in position
  • Position.withdraw() requires FungibleToken.Withdraw authorization that transactions cannot obtain
  • This made the protocol unusable for real lending scenarios

Solution

Added repayAndClosePosition() method to the Pool contract that:

  1. Accepts MOET repayment vault
  2. Deposits repayment to clear debt
  3. Internally withdraws all collateral (using contract's authorization)
  4. Returns collateral to user-provided receiver capabilities
  5. Verifies position has no remaining balances

Changes

  • Added repayAndClosePosition() method to TidalProtocol.Pool
  • Added getId() method to Position struct to expose position ID
  • Created repay_and_close_position_v2.cdc transaction using the new method
  • Updated position_lifecycle_happy_test.cdc to verify collateral return

Test Results

✅ All tests pass with collateral successfully returned:

Position ID: 0
=== Position Details BEFORE Repayment ===
Token: A.0000000000000007.MOET.Vault | Direction: Debit | Amount: 615.38461538
Token: A.0000000000000003.FlowToken.Vault | Direction: Credit | Amount: 1000.00000000

=== Using Contract Method (repayAndClosePosition) ===
=== Position Successfully Closed ===
MOET debt repaid: 615.38461538
Flow collateral returned to user!
User's Flow balance after closing: 1000.00000000

Testing

Run: flow test cadence/tests/position_lifecycle_happy_test.cdc

This PR is stacked on top of #14 and completes the position lifecycle functionality.

- Added repayAndClosePosition() method to Pool contract that handles both debt repayment and collateral return

- Added getId() method to Position struct to expose position ID

- Created repay_and_close_position_v2.cdc transaction that uses the new contract method

- Updated position_lifecycle_happy_test.cdc to verify collateral is returned

This fixes the critical issue where users could not retrieve their collateral after repaying loans.

The contract method has internal access to withdraw funds, bypassing the FungibleToken.Withdraw authorization issue.

Test now shows:

- MOET debt: 615.38 → 0 (repaid)

- Flow balance: 0 → 1000 (collateral returned)

- Position successfully closed
CRITICAL: The repayAndClosePosition method allows anyone to close anyone's position

This must be fixed before production deployment
@kgrgpg
Copy link
Contributor Author

kgrgpg commented Jun 19, 2025

⚠️ CRITICAL SECURITY ISSUE DISCOVERED ⚠️

During review, I realized that the repayAndClosePosition() method has NO ownership verification. This means anyone can close anyone else's position and steal their collateral.

I've documented this in SECURITY_ISSUE_OWNERSHIP.md with proposed solutions.

This MUST be fixed before any production deployment.

The test passes because it's only testing the happy path where the position owner closes their own position. But in reality, any address could call this method with any position ID.

Proposed fixes:

  1. Add owner tracking to InternalPosition
  2. Require Position struct as proof of ownership
  3. Move the method to Position struct itself

For now, this PR demonstrates the solution to the collateral withdrawal problem, but it introduces a critical security vulnerability that needs immediate attention.

kgrgpg added a commit that referenced this pull request Jun 19, 2025
- Merged PR #17 which provides a clean solution for collateral withdrawal

- Uses MockTidalProtocolConsumer.borrowPositionForWithdraw() to provide FungibleToken.Withdraw authorization

- withdrawAndPull with pullFromTopUpSource: true automatically repays debt and returns collateral

- Updated misleading comments and logs to reflect that collateral IS successfully returned

- This approach avoids the security vulnerability in PR #16's repayAndClosePosition method
@kgrgpg
Copy link
Contributor Author

kgrgpg commented Jun 19, 2025

Suggestion: Close this PR in favor of PR #14

I've merged Kay-Zee's solution from PR #17 into PR #14, which provides a better approach to the collateral withdrawal issue without the security vulnerability present in this PR.

The solution in PR #14:

  • Uses the existing withdrawAndPull mechanism with pullFromTopUpSource: true
  • Doesn't require contract changes
  • Is secure - only the position owner can perform the operation
  • Avoids the critical vulnerability where anyone could close anyone else's position

Since PR #14 now includes a complete and secure solution for the position lifecycle (including collateral withdrawal), I suggest closing this PR to avoid the security risk.

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