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.

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

Audit followup refactor #88

Merged
merged 20 commits into from
Jun 24, 2024
Merged

Audit followup refactor #88

merged 20 commits into from
Jun 24, 2024

Conversation

sisyphusSmiling
Copy link
Contributor

@sisyphusSmiling sisyphusSmiling commented Jun 21, 2024

Closes: #79

Note: Continuing from #87 after cherry picking & consolidating commit to individual finding resolution as requested by auditing team.


For contributor use:

  • Targeted PR against main branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the standards mentioned here.
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@sisyphusSmiling sisyphusSmiling self-assigned this Jun 21, 2024
@sisyphusSmiling sisyphusSmiling marked this pull request as ready for review June 21, 2024 00:20
@sisyphusSmiling sisyphusSmiling requested a review from a team as a code owner June 21, 2024 00:20
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 89.39394% with 7 lines in your changes missing coverage. Please review.

Project coverage is 86.61%. Comparing base (02aab3f) to head (7092bb7).
Report is 13 commits behind head on main.

Files Patch % Lines
cadence/contracts/bridge/FlowEVMBridgeConfig.cdc 88.57% 4 Missing ⚠️
cadence/contracts/utils/SerializeMetadata.cdc 81.81% 2 Missing ⚠️
...adence/contracts/bridge/FlowEVMBridgeNFTEscrow.cdc 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #88      +/-   ##
==========================================
+ Coverage   86.19%   86.61%   +0.41%     
==========================================
  Files          18       17       -1     
  Lines         884      919      +35     
==========================================
+ Hits          762      796      +34     
- Misses        122      123       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

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

Just a few comments! Looks pretty good

/// Flag indicating whether operations for the associated Type are paused
access(all) var isPaused: Bool

init(associated evmAddress: EVM.EVMAddress) {
Copy link
Member

@joshuahannan joshuahannan Jun 24, 2024

Choose a reason for hiding this comment

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

Maybe I am just blanking and forgetting a cadence syntax thing here, but what does associated evmAddress mean? looks like incorrect syntax. 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cadence has support for argument labeling, similar to Swift though it's not well documented in the lang ref. This same question was raised by the auditors during their review, so maybe I should just remove this pattern.

@@ -29,13 +30,15 @@ contract FlowEVMBridgeConfig {
/// Default ERC20.decimals() value
access(all)
let defaultDecimals: UInt8
access(all)
var gasLimit: UInt64
Copy link
Member

Choose a reason for hiding this comment

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

I see that we're using this everywhere now instead of before having different gas limits for different types of operations. Correct me if I'm wrong, but I thought that in EVM, the gas limit is completely consumed regardless of whether or not the transaction actually needed it or not, so would it make more sense to have different gas limits for different types of transactions? That way, we could make sure they are all only using as much as is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm good callout, let me do some digging here. So far I've used the gas limit value as the maximum computation allowable for a given operation, which while the final computation used is a factor in fee calculation is not necessarily the basis for fees which have their own rules in relation to operations in Cadence. So from my assumptions, I've considered the gasLimit is the upper bound for how much computation is allowable for a COA call. If it's true that the gasLimit is in fact defining a set fee amount for the execution of the call or even the gas that is exactly to be used by that call, then you're right that should be updated for more granular control of user fees but I need to double check my understanding.

Copy link
Contributor Author

@sisyphusSmiling sisyphusSmiling Jun 24, 2024

Choose a reason for hiding this comment

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

From running a quick EVM deployment transaction in emulator with this gas limit, I can see that the gas limit seems to be an upper bound as the gas consumed is much less than the provided gas limit value. In this case, I don't necessarily see the need for a more granular gas limit value, especially since the fees are ultimately all paid via the wrapping flow transaction - callers could always add a gas limit to the Cadence transaction which would limit the total gas used for the entire transaction. What do you think @joshuahannan?

Update: In the interest of getting these updates back to the auditing team asap, I'll merge the changes as approved, but still curious about any remaining thoughts you may have on this.

let postRegistrarResult = self.coa.call(
to: registryEVMAddress,
data: EVM.encodeABIWithSignature("registrar()", []),
gasLimit: 15_000_000,
Copy link
Member

Choose a reason for hiding this comment

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

If we're using the gas limit constant in the contract, should we try to use it in transactions also?

Copy link
Contributor Author

@sisyphusSmiling sisyphusSmiling Jun 24, 2024

Choose a reason for hiding this comment

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

Yeah good point, will follow up in separate PR with changes to transactions using that contract value.

@sisyphusSmiling sisyphusSmiling merged commit 5db91fa into main Jun 24, 2024
2 checks passed
This was referenced Jun 24, 2024
@sisyphusSmiling sisyphusSmiling deleted the audit-followup-refactor branch September 19, 2024 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Respond to audit results
3 participants