Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added documentation and used transfer_checked instead #254

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

donjne
Copy link

@donjne donjne commented Nov 2, 2024

Add Documentation and Implement transfer_checked

Changes

  1. Implemented transfer_checked for improved token transfer safety

    • Replaced transfer with transfer_checked in token operations
    • Added decimal verification during transfers
    • Added mint validation during transfers
  2. Enhanced documentation across multiple components:

    • Added comprehensive struct-level documentation
    • Improved method-level documentation
    • Added detailed security considerations
    • Clarified terminology (underlying tokens/collateral)
    • Added usage examples and scenarios

Security Considerations

  • transfer_checked provides additional runtime safety by verifying:
    • Token mint addresses match expected values
    • Decimal places are consistent
    • Token accounts belong to correct mint
  • Additional validation checks prevent token substitution attacks

Comment on lines +81 to +91
// Validate token accounts and decimals
require_eq!(
accs.user_underlying_token_account.mint,
accs.vault_underlying_token_mint.key(),
VaultError::UnderlyingTokenMintMismatch
);
require_eq!(
accs.vault_underlying_token_account.mint,
accs.vault_underlying_token_mint.key(),
VaultError::UnderlyingTokenMintMismatch
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are already checked in InteractWithVault

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are already checked in InteractWithVault

Yes I performed those validations before leveraging the usefulness of TransferChecked and it seems kind of redundant. But even with that, TransferChecked remains a good option.

CpiContext::new_with_signer(
accs.token_program.to_account_info(),
Transfer {
TransferChecked {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of TransferChecked if we already know that the mints are the same?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#254 (comment)

I replied to it above. But if you're already sure about the mint and decimals, then you can use Transfer.

@metaproph3t
Copy link
Member

Thanks for taking a look and submitting this. Two things:

  • There should be one PR per thing done rather than bundled
  • I don't see the benefit of these changes, you can find a list of outstanding issues here

@donjne
Copy link
Author

donjne commented Nov 6, 2024

Yes

Thanks for taking a look and submitting this. Two things:

  • There should be one PR per thing done rather than bundled
  • I don't see the benefit of these changes, you can find a list of outstanding issues here

I will make sure future PRs are made per thing.
It's not much yeah. Will check out the issues.

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.

2 participants