p-token: Simplify math operations #83
Conversation
| @@ -8,6 +8,7 @@ use { | |||
| }; | |||
|
|
|||
| #[inline(always)] | |||
| #[allow(clippy::arithmetic_side_effects)] | |||
There was a problem hiding this comment.
nit: If you want to keep the clippy check, we can change the code to wrapping_*, but then it will also wrap for test builds
There was a problem hiding this comment.
Maybe we should keep it as it is then? It would be useful to panic on tests, although asserts should also pick up any issues.
| // Moves the lamports to the destination account. | ||
| *destination_account_info.borrow_mut_lamports_unchecked() = destination_starting_lamports | ||
| .checked_add(source_account_info.lamports()) | ||
| .ok_or(TokenError::Overflow)?; | ||
| *destination_account_info.borrow_mut_lamports_unchecked() += source_account_info.lamports(); |
There was a problem hiding this comment.
Can you add a comment about why this is safe?
To be honest, I'm not 100% sure about this one... if someone artificially sets a huge amount on source_account_info before CPI, is it possible to wrap around and set lower lamports on destination_account_info? And then after CPI, set the numbers back to normal.
Edit: assuming destination_account_info is owned by the token program
There was a problem hiding this comment.
Do you mean the lamports on the account? The runtime will check whether lamports of accounts are balanced or not when you CPI, so I don't think it is possible to artificially "inflate" the lamports – the amount needs to come from another account, so in this case it will be within the lamports supply (u64) limit.
My "new" understanding is that it is safe to add lamports to an account, since it always needs to come from another account and the resulting value will always be within u64 limit.
There was a problem hiding this comment.
Yes, the runtime does check that the total sum among all instruction accounts stays constant. However, it can not guarantee that the lamports came from the accounts you wanted them to come from. That is for the program to make sure. So, maybe there is a scenario where I can use overflows in your program to trick it into subtracting them from the wrong source?
There was a problem hiding this comment.
But when you are moving lamports around, it will always be within the u64 limit right?
There was a problem hiding this comment.
@joncinque Wondering if we should keep this change or revert it? My understanding is that it is safe to keep it.
There was a problem hiding this comment.
Let's try an example and see...
Let's say we have a source wrapped SOL account with 10 SOL, and a close destination wrapped SOL account with 20 SOL.
If a malicious program sets the lamports in the close destination to u64::MAX - 5 SOL, then CPIs into close_account, then the destination will be set to 5 SOL, and the source token account will be set to 0. Afterwards, I can move those 15 "disappeared" SOL wherever I want.
Does this work though? I think the runtime should check after the CPI to make sure the instruction is balanced, and it should notice all of the SOL that disappeared from the destination.
Maybe it's worth writing a test to check this, unless we're sure that this behavior isn't possible.
febo
force-pushed
the
febo/simplify-math
branch
from
2edf20b to
9e62a06
Compare
Problem
Following #82, there are a few more places where math operations can be simplified since it is guaranteed that values will be within the valid limit.
Solution
Simplify math operation, which leads to reduction in CU.