code-423n4 / 2024-03-phala-network-findings

0 stars 0 forks source link

QA Report #11

Open c4-bot-4 opened 4 months ago

c4-bot-4 commented 4 months ago

See the markdown file with the details of this report here.

c4-pre-sort commented 3 months ago

141345 marked the issue as high quality report

141345 commented 3 months ago

11 Cryptor l r nc 3 0 11

L 1 i L 2 n L 3 n L 4 n L 5 n L 6 n L 7 i L 8 n L 9 l L 10 l L 11 l L 12 n L 13 n L 14 n L 15 n L 16 n

kvinwang commented 3 months ago

[L01] Ensure_System should be checked after uploading bare code ❌,The system code is fetched on-chain by the worker.

[L02] No point in using regular http request over batchhttperequest ❌,len = 1 is a valid batch.

[L03] CallinCommand is misleading and should be renamed callinTransaction for clarity ✅

[L04] is_running_in_command function is missing or misnamed in the code ✅

[L05] Functions that are not supported in transaction mode should revert not send empty arrays Judgable. It did actually revert in the past, but the reverting can not be catched by the contract which leads bad dev experience.

[L06] Twox64Concat has a higher risk of a hash collision than blake128Concat ❌,The key itself if already crypto hash.

[L07] Ensure_System should be called for sign and verify ❌,The sign and verify are nothing about system contract.

[L08] fn setup cannot be called in estimating mode ❌,There is no code path in estimating mode would call this function.

[L09] Masking deposit calculation can lead to unexpected results in certain circumstances If deposit_per_byte is = 664613997892457936451903530140172289, the cluster wouldn't be able to do anything as it is two expensive.

[L10] Not checking the bytecode for system contract can be dangerous as it can be upgraded ❌,The system code upgrading is managed by on-chain goverance.

[L11] No deadline for signature function ❌,Chain extenstions is the low level API, higher level sdk can to the work.

[L12] is_in_transaction return value can be misleading ✅,this is actually on the plan.

[L13] MAX_CODE_LEN should equal the schedule ❌,schedule.payload_len is a config for different thing

[L14] No code size limit check in function put_sidevm_code ✅,might good to have. However, there is already a on-chain limit check for that.

[L15] Omission of reserved balance ❌,As total = free + reserved, the reserved can be calculated external or in the higher level SDK.

[L16] Payer can be any account ❌,It is called by the trusted worker.

c4-sponsor commented 3 months ago

kvinwang (sponsor) confirmed

c4-judge commented 3 months ago

OpenCoreCH marked the issue as grade-a

c4-judge commented 3 months ago

OpenCoreCH marked the issue as selected for report

OpenCoreCH commented 3 months ago

Agree with the Lookout's assessment of L / NC and the sponsors assessment of the validity with the following differences: L02: Valid design suggestion (not necessarily better than the current one, but just an alternative one) L05: Also a design suggestion

thebrittfactor commented 3 months ago

Just a note that C4 is excluding the invalid entries from the official report.

For confirmation, the judge provided the following comment via DM:

to summarize, invalid are: L-01, L-06, L-07, L-08, L-10, L11, L-13, L-15, L-16