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.

Sign up for GitHub

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

Add validation preventing caller PDA to be garbage-collected #386

Merged
merged 1 commit into from
Dec 19, 2024
Merged

Add validation preventing caller PDA to be garbage-collected #386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions chains/solana/contracts/programs/ccip-router/src/fee_quoter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,12 @@ pub fn wrap_native_sol<'info>(
amount: u64,
signer_bump: u8,
) -> Result<()> {
require!(
// guarantee that if caller is a PDA it won't get garbage-collected
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be incorrect to check this always? Even if not a PDA?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH, it likely doesn't matter. My personal take is that yes, it would be incorrect, though the impact of it is likely very small.

The rent exemption only matters when accounts hold data. When the owner of that account withdraws all funds, leaving it with a 0 balance, the account is closed and the data is removed. This check aims to prevent that.

However, if the caller does not hold any data (e.g. it is just a wallet), then there is no reason to prevent them spending their last lamports in invoking ccip_send. Technically speaking, the check about the system program owning it is not exactly the same as checking whether the account holds data, as it is possible to have dataless PDAs, but it is close enough for an edge case and should still protect agains all actual "dataful" accounts being closed (even if we're failing in a corner case for a dataless PDA when we shouldn't).

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. Agree. There's still a chance that the user's account is closed after calling ccip_send and some one else then gets ownership of the same account. Anyways, that's the same behavior that transferring tokens has, so not a concern.

*from.owner == System::id() || from.get_lamports() > amount,
CcipRouterError::InsufficientLamports
);

invoke_signed(
&system_instruction::transfer(&from.key(), &to.key(), amount),
&[from.to_account_info(), to.to_account_info()],
Expand Down
2 changes: 2 additions & 0 deletions chains/solana/contracts/programs/ccip-router/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1601,6 +1601,8 @@ pub enum CcipRouterError {
InvalidTokenPrice,
#[msg("Stale gas price")]
StaleGasPrice,
#[msg("Insufficient lamports")]
InsufficientLamports,
}

// TODO: Refactor this to use the same structure as messages: execution_report.validate(..)
Expand Down
3 changes: 3 additions & 0 deletions chains/solana/contracts/target/idl/ccip_router.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2726,6 +2726,9 @@
},
{
"name": "StaleGasPrice"
},
{
"name": "InsufficientLamports"
}
]
}
Expand Down
2 changes: 1 addition & 1 deletion chains/solana/contracts/tests/ccip/ccip_router_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2785,7 +2785,7 @@ func TestCCIPRouter(t *testing.T) {
}
})

t.Run("hen sending a Valid CCIP Message but the user does not have enough funds of the fee token, it fails", func(t *testing.T) {
t.Run("When sending a Valid CCIP Message but the user does not have enough funds of the fee token, it fails", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a test for this specific case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it initially, but that requires me to use another program as the caller (so that it's a PDA) and guarantee that the balance is just right. It seems like an overkill to create a new program (or extend the ccip-receiver) just to test this fairly basic check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let's avoid having more ccip receiver examples

message := ccip_router.Solana2AnyMessage{
FeeToken: token2022.mint,
Receiver: validReceiverAddress[:],
Expand Down
3 changes: 3 additions & 0 deletions chains/solana/gobindings/ccip_router/types.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading