-
Notifications
You must be signed in to change notification settings - Fork 72
refactor(security-practices): port security article code samples to Tolk #2134
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,35 +10,33 @@ Improper handling of signed integers can allow attackers to exploit overflow/und | |
|
|
||
| ### Vulnerable code | ||
|
|
||
| ```func | ||
| (cell,()) transfer_voting_power(cell votes, slice from, slice to, int amount) impure { | ||
| int from_votes = get_voting_power(votes, from); | ||
| int to_votes = get_voting_power(votes, to); | ||
|
|
||
| from_votes -= amount; // Can become negative! | ||
| to_votes += amount; | ||
|
|
||
| votes~set_voting_power(from, from_votes); | ||
| votes~set_voting_power(to, to_votes); | ||
| return (votes,()); | ||
| ```tolk | ||
| fun transferVotingPower(mutate votes: cell, from: slice, to: slice, amount: int): void { | ||
| var fromVotes = getVotingPower(votes, from); | ||
| var toVotes = getVotingPower(votes, to); | ||
|
|
||
| fromVotes -= amount; // Can become negative | ||
| toVotes += amount; | ||
|
|
||
| votes.setVotingPower(from, fromVotes); | ||
| votes.setVotingPower(to, toVotes); | ||
| } | ||
| ``` | ||
|
|
||
| ### Secure implementation | ||
|
|
||
| ```func | ||
| (cell,()) transfer_voting_power(cell votes, slice from, slice to, int amount) impure { | ||
| int from_votes = get_voting_power(votes, from); | ||
| int to_votes = get_voting_power(votes, to); | ||
|
|
||
| throw_unless(998, from_votes >= amount); // Validate sufficient balance | ||
|
|
||
| from_votes -= amount; | ||
| to_votes += amount; | ||
|
|
||
| votes~set_voting_power(from, from_votes); | ||
| votes~set_voting_power(to, to_votes); | ||
| return (votes,()); | ||
| ```tolk | ||
| fun transferVotingPower(mutate votes: cell, from: slice, to: slice, amount: int): void { | ||
| var fromVotes = getVotingPower(votes, from); | ||
| var toVotes = getVotingPower(votes, to); | ||
|
|
||
| assert (fromVotes >= amount) throw 998; // Validate sufficient balance | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This secure version still accepts a negative |
||
|
|
||
| fromVotes -= amount; | ||
| toVotes += amount; | ||
|
|
||
| votes.setVotingPower(from, fromVotes); | ||
| votes.setVotingPower(to, toVotes); | ||
| } | ||
| ``` | ||
|
|
||
|
|
@@ -48,12 +46,12 @@ The entire smart contract computation is transparent; confidential runtime value | |
|
|
||
| ### Vulnerable code | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs a note saying that important private data should not be sent whatsoever. That is, "secure approach" = "don't". |
||
|
|
||
| ```func | ||
| ;; DON'T: Storing password hash or private data | ||
| cell private_data = begin_cell() | ||
| .store_slice("secret_password_hash") | ||
| .store_uint(user_private_key, 256) | ||
| .end_cell(); | ||
| ```tolk | ||
| // DON'T: Store password hash or private data | ||
| val privateData = beginCell() | ||
| .storeSlice("secret_password_hash") | ||
| .storeUint(userPrivateKey, 256) | ||
| .endCell(); | ||
| ``` | ||
|
aigerimu marked this conversation as resolved.
|
||
|
|
||
| ## Account destruction race conditions | ||
|
|
@@ -62,28 +60,28 @@ Destroying accounts using [send mode](/foundations/messages/modes) `128 + 32` wi | |
|
|
||
|
aigerimu marked this conversation as resolved.
|
||
| ### Vulnerable code | ||
|
|
||
| ```func | ||
| () recv_internal(msg_value, in_msg_full, in_msg_body) { | ||
| if (in_msg_body.slice_empty?()) { | ||
| return (); ;; Dangerous: empty message handling | ||
| ```tolk | ||
| fun onInternalMessage(msgValue: int, inMsgFull: cell, inMsgBody: slice) { | ||
| if (inMsgBody.isEmpty()) { | ||
| return; // Dangerous: empty message handling | ||
| } | ||
| ;; Process and destroy account | ||
| send_raw_message(msg, 128 + 32); ;; Destroys account | ||
|
|
||
| // Process and destroy account | ||
| sendRawMessage(msg, 128 + 32); // Destroys account | ||
|
aigerimu marked this conversation as resolved.
Outdated
|
||
| } | ||
|
aigerimu marked this conversation as resolved.
aigerimu marked this conversation as resolved.
Outdated
|
||
| ``` | ||
|
|
||
| ### Secure approach | ||
|
|
||
| ```func | ||
| () recv_internal(msg_value, in_msg_full, in_msg_body) { | ||
| ;; Proper validation before any destruction | ||
| throw_unless(error::unauthorized, authorized_sender?(sender)); | ||
| ;; Ensure no pending operations | ||
| throw_unless(error::pending_operations, safe_to_destroy?()); | ||
| ;; Then proceed with destruction if really needed | ||
| ```tolk | ||
| fun onInternalMessage(msgValue: int, inMsgFull: cell, inMsgBody: slice) { | ||
| // Proper validation before any destruction | ||
| assert (authorizedSender(sender)) throw ErrCode.Unauthorized; | ||
|
|
||
| // Ensure no pending operations | ||
| assert (safeToDestroy()) throw ErrCode.PendingOperations; | ||
|
|
||
| // Then proceed with destruction if really needed | ||
| } | ||
| ``` | ||
|
|
||
|
|
@@ -93,18 +91,22 @@ Replay protection is a security mechanism that prevents an attacker from [reusin | |
|
|
||
| ### Secure implementation | ||
|
|
||
| ```func | ||
| () recv_external(slice in_msg) impure { | ||
| slice ds = get_data().begin_parse(); | ||
| int stored_seqno = ds~load_uint(32); | ||
| int msg_seqno = in_msg~load_uint(32); | ||
|
|
||
| throw_unless(33, msg_seqno == stored_seqno); ;; Prevent replay | ||
|
|
||
| accept_message(); | ||
|
|
||
| ;; Update sequence number | ||
| set_data(begin_cell().store_uint(stored_seqno + 1, 32).end_cell()); | ||
| ```tolk | ||
| fun onExternalMessage(mutate inMsg: slice) { | ||
| var ds = contract.getData().beginParse(); | ||
| val storedSeqno = ds.loadUint(32); | ||
| val msgSeqno = inMsg.loadUint(32); | ||
|
|
||
| assert (msgSeqno == storedSeqno) throw 33; // Prevent replay | ||
|
|
||
| acceptMessage(); | ||
|
|
||
|
aigerimu marked this conversation as resolved.
|
||
| // Update sequence number | ||
| contract.setData( | ||
| beginCell() | ||
| .storeUint(storedSeqno + 1, 32) | ||
| .endCell() | ||
| ); | ||
| } | ||
| ``` | ||
|
|
||
|
|
@@ -120,25 +122,25 @@ The [`SETGASLIMIT`](/tvm/instructions#f801-setgaslimit) instruction can lead to | |
|
|
||
| ### Vulnerable code | ||
|
|
||
| ```func | ||
| () recv_external(slice in_msg) { | ||
| accept_message(); | ||
| ;; ... | ||
| ```tolk | ||
| fun onExternalMessage(inMsg: slice) { | ||
| acceptMessage(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
this appears again in the secure example below at line 142, so both examples should use |
||
| // ... | ||
| } | ||
| ``` | ||
|
|
||
| ### Secure implementation | ||
|
|
||
| Checks before accepting an external message vary by use case. The following example is a part of [wallet v3 code](https://github.com/ton-blockchain/ton/blob/53ec9684bd213983e1fe0f7610d3e3453a4ec628/crypto/smartcont/wallet3-code.fc), that doesn't accept a message if it isn't signed by the wallet's owner. | ||
|
|
||
| ```func | ||
| () recv_external(slice in_msg) impure { | ||
| ;; parse message and contract storage (omitted) | ||
| throw_unless(33, msg_seqno == stored_seqno); | ||
| throw_unless(34, subwallet_id == stored_subwallet); | ||
| throw_unless(35, check_signature(slice_hash(in_msg), signature, public_key)); | ||
| accept_message(); | ||
| ;; handle the message (omitted) | ||
| ```tolk | ||
| fun onExternalMessage(inMsg: slice) { | ||
| // parse message and contract storage (omitted) | ||
| assert (msgSeqno == storedSeqno) throw 33; | ||
| assert (subwalletId == storedSubwallet) throw 34; | ||
| assert (checkSignature(inMsg.hash(), signature, publicKey)) throw 35; | ||
|
aigerimu marked this conversation as resolved.
Outdated
|
||
| acceptMessage(); | ||
| // handle the message (omitted) | ||
| } | ||
| ``` | ||
|
aigerimu marked this conversation as resolved.
|
||
|
|
||
|
|
@@ -152,20 +154,21 @@ Be careful with the [Out of gas error](/tvm/exit-codes#13%3A-out-of-gas-error). | |
|
|
||
| ### Secure implementation | ||
|
|
||
| ```tact | ||
| message Vote { votes: Int as int32 } | ||
| ```tolk | ||
| struct Vote { | ||
| votes: int32 | ||
| } | ||
|
|
||
| contract VoteCounter() { | ||
| const voteGasUsage = 10000; // precompute with tests | ||
| const voteGasUsage = 10000; // precompute with tests | ||
|
|
||
| receive(msg: Vote) { | ||
| require(context().value > getComputeFee(self.voteGasUsage, false), "Not enough gas!"); | ||
| // ...subsequent logic... | ||
| } | ||
| fun onInternalMessage(in: InMessage) { | ||
| val msg = Vote.fromSlice(in.body); | ||
|
|
||
| // Empty receiver for the deployment, | ||
| // which forwards the remaining value back to the sender | ||
| receive() { cashback(sender()) } | ||
| assert ( | ||
| in.valueCoins > getComputeFee(voteGasUsage, false) | ||
| ) throw ErrCode.NotEnoughGas; // Not enough gas! | ||
|
|
||
| // ...subsequent logic... | ||
| } | ||
| ``` | ||
|
aigerimu marked this conversation as resolved.
|
||
|
|
||
|
|
@@ -193,10 +196,10 @@ Executing untrusted code can compromise contract security. | |
|
|
||
| ### Prevention | ||
|
|
||
| ```func | ||
| ;; Validate all external code before execution | ||
| throw_unless(error::untrusted_code, verify_code_signature(code)); | ||
| throw_unless(error::invalid_code, validate_code_safety(code)); | ||
| ```tolk | ||
| // Validate all external code before execution | ||
| assert (verifyCodeSignature(code)) throw ErrCode.UntrustedCode; | ||
| assert (validateCodeSafety(code)) throw ErrCode.InvalidCode; | ||
| ``` | ||
|
aigerimu marked this conversation as resolved.
|
||
|
|
||
| ## Race condition of messages | ||
|
|
@@ -386,13 +389,13 @@ contract AnotherContract( | |
|
|
||
| ### Address formats | ||
|
|
||
| ```func | ||
| ;; Raw: 0:b4c1b2ede12aa76f4a44353944258bcc8f99e9c7c474711a152c78b43218e296 | ||
| ;; Bounceable: EQC0wbLt4Sqnb0pENTlEJYvMj5npx8R0cRoVLHi0MhjilkPX | ||
| ;; Non-bounceable: UQC0wbLt4Sqnb0pENTlEJYvMj5npx8R0cRoVLHi0Mhjilh4S | ||
| ```tolk | ||
| // Raw: 0:b4c1b2ede12aa76f4a44353944258bcc8f99e9c7c474711a152c78b43218e296 | ||
| // Bounceable: EQC0wbLt4Sqnb0pENTlEJYvMj5npx8R0cRoVLHi0MhjilkPX | ||
| // Non-bounceable: UQC0wbLt4Sqnb0pENTlEJYvMj5npx8R0cRoVLHi0Mhjilh4S | ||
|
|
||
| ;; Always validate workchain | ||
| force_chain(to_address); | ||
| // Always validate workchain | ||
| forceChain(toAddress); | ||
| ``` | ||
|
|
||
| ## Name collision vulnerabilities | ||
|
|
@@ -401,10 +404,10 @@ Function or variable names can collide with built-in functions or reserved keywo | |
|
|
||
| ### Best practice | ||
|
|
||
| ```func | ||
| ;; Use descriptive, unique names | ||
| int user_balance = 0; ;; Instead of just 'balance' | ||
| () validate_user_signature() ;; Instead of just 'validate()' | ||
| ```tolk | ||
| // Use descriptive, unique names | ||
| var userBalance = 0; // Instead of just `balance` | ||
| fun validateUserSignature() { } // Instead of just `validate()` | ||
| ``` | ||
|
|
||
| ## Incorrect data type handling | ||
|
|
@@ -413,18 +416,26 @@ Reading or writing incorrect data types can corrupt contract state. | |
|
|
||
| ### Vulnerable code | ||
|
|
||
| ```func | ||
| ;; Writing uint but reading int | ||
| storage~store_uint(value, 32); | ||
| int read_value = storage~load_int(32); ;; Type mismatch | ||
| ```tolk | ||
| // Writing uint but reading int | ||
| val data = beginCell() | ||
| .storeUint(value, 32) | ||
| .endCell(); | ||
|
|
||
| var storage = data.beginParse(); | ||
| val readValue = storage.loadInt(32); // Type mismatch | ||
|
aigerimu marked this conversation as resolved.
|
||
| ``` | ||
|
|
||
| ### Secure implementation | ||
|
|
||
| ```func | ||
| ;; Consistent type usage | ||
| storage~store_uint(value, 32); | ||
| int read_value = storage~load_uint(32); | ||
| ```tolk | ||
| // Consistent type usage | ||
| val data = beginCell() | ||
| .storeUint(value, 32) | ||
| .endCell(); | ||
|
|
||
| var storage = data.beginParse(); | ||
| val readValue = storage.loadUint(32); | ||
| ``` | ||
|
|
||
| ## Missing function return value checks | ||
|
|
@@ -434,14 +445,14 @@ Ignoring function return values can lead to logic errors and unexpected behavior | |
| ### Vulnerable code: | ||
|
|
||
| ```func | ||
|
aigerimu marked this conversation as resolved.
Outdated
|
||
| dictinfos~udict_delete?(32, index); ;; Ignoring success flag | ||
| infoDict.delete(index); // Ignoring success flag | ||
| ``` | ||
|
|
||
| ### Secure implementation: | ||
|
|
||
| ```func | ||
| int success = dictinfos~udict_delete?(32, index); | ||
| throw_unless(error::fail_to_delete_dict, success); | ||
| ```tolk | ||
| val success = infoDict.delete(index); | ||
| assert (success) throw ErrCode.FailToDeleteDict; | ||
| ``` | ||
|
|
||
| ## Contract code updates | ||
|
|
@@ -450,11 +461,11 @@ Contracts can be updated if not properly protected, changing their behavior unex | |
|
|
||
|
aigerimu marked this conversation as resolved.
|
||
| ### Secure implementation | ||
|
|
||
| ```func | ||
| () update_code(cell new_code) impure { | ||
| throw_unless(error::unauthorized, authorized_admin?(sender())); | ||
| throw_unless(error::invalid_code, validate_code?(new_code)); | ||
| set_code(new_code); | ||
| ```tolk | ||
| fun updateCode(newCode: cell) { | ||
| assert (authorizedAdmin(sender())) throw ErrCode.Unauthorized; | ||
|
aigerimu marked this conversation as resolved.
Outdated
|
||
| assert (validateCode(newCode)) throw ErrCode.InvalidCode; | ||
|
|
||
| setCode(newCode); | ||
|
aigerimu marked this conversation as resolved.
Outdated
|
||
| } | ||
| ``` | ||
Uh oh!
There was an error while loading. Please reload this page.