-
Notifications
You must be signed in to change notification settings - Fork 154
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
Certik Audit Recommendations #151
Conversation
Looks good. Thanks 👍 |
@@ -90,7 +90,6 @@ contract Orchestrator is Ownable { | |||
transactions[index] = transactions[transactions.length - 1]; | |||
} | |||
|
|||
delete transactions[transactions.length - 1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's hidden and good to know! we have this delete pattern in multiple places in our codebase.
@@ -147,9 +144,9 @@ contract Orchestrator is Ownable { | |||
|
|||
|
|||
destination, | |||
transferValueWei, | |||
0, // transfer value in wei |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we already did this when I left a comment for it in the original PR.
Exhibit 1-
Remove Orchestrator address initialization in the policy.
Exhibit 2-
Reorder Transaction struct elements to pack values and save storage.
Exhibit 3-
No action. I'm ok with leaving the index in the
TransactionFailed
event, since risk is minimal and it aids potential debugging.Exhibit 4-
No action needed.
Exhibit 5-
Removed unnecessary
delete
call.Exhibit 6-
Removed
transferValueWei
parameter in external_call.Exhibit 7-
Remove
dataLength
parameter in external_call. Load directly from array's storage.Exhibit 8-
No action. It doesn't save gas, and I think it's more legible as is.
Exhibit 9-
Renamed
transactionsLength
totransactionsSize
to follow market-oracle repo's convention.Exhibit 10-
No action needed, since we mitigated Exhibit 2.