Audit followup refactor #88
Conversation
…ig & fix escrow overflow
Codecov ReportAttention: Patch coverage is
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. |
| /// Flag indicating whether operations for the associated Type are paused | ||
| access(all) var isPaused: Bool | ||
|
|
||
| init(associated evmAddress: EVM.EVMAddress) { |
There was a problem hiding this comment.
Maybe I am just blanking and forgetting a cadence syntax thing here, but what does associated evmAddress mean? looks like incorrect syntax. 🤷
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
If we're using the gas limit constant in the contract, should we try to use it in transactions also?
There was a problem hiding this comment.
Yeah good point, will follow up in separate PR with changes to transactions using that contract value.
Closes: #79
Note: Continuing from #87 after cherry picking & consolidating commit to individual finding resolution as requested by auditing team.
For contributor use:
mainbranchFiles changedin the Github PR explorer