Fix/finding #21 - Emit Events for Failed Try Execution in execute
#127
Conversation
…tests to TestAccountExecution_TryExecuteSingle.t.sol
Aboudjem
changed the title
executeexecute
Aboudjem
requested review from
filmakarov,
ankurdubey521,
VGabriel45 and
livingrockrises
Fix/finding #21 - Emit Events for Failed Try Execution in
|
| Severity Level | Results | |
|---|---|---|
| Contracts | Critical High Medium Low Note Total |
0 1 0 6 24 31 |
For more details view the full report in OpenZeppelin Code Inspector
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## fix/finding-19-enable-mode-validator-check #127 +/- ##
==============================================================================
- Coverage 72.68% 72.54% -0.14%
==============================================================================
Files 13 13
Lines 659 663 +4
Branches 149 151 +2
==============================================================================
+ Hits 479 481 +2
- Misses 180 182 +2
Continue to review full report in Codecov by Sentry.
|
contracts/base/ExecutionHelper.sol
Outdated
| else revert UnsupportedExecType(execType); | ||
| else if (execType == EXECTYPE_TRY) { | ||
| (bool success, bytes memory result) = _tryExecute(target, value, callData); | ||
| if (!success) emit TryExecuteUnsuccessful(0, result); |
There was a problem hiding this comment.
I had also gotten feedback that the information we emit for these internal success or failure got to make more sense. for example it should emit calldata and not emit things like indexes
I will share the finding description
There was a problem hiding this comment.
why there are two prs with same work?
| @@ -16,6 +16,7 @@ contract EventsAndErrors { | |||
| event PreCheckCalled(); | |||
| event PostCheckCalled(); | |||
| event TryExecuteUnsuccessful(uint256 batchExecutionindex, bytes result); | |||
There was a problem hiding this comment.
we should rethink what should be emitted..
There was a problem hiding this comment.
this is updated right?
Should we close this since Gabi has opened an PR for the same? |
🤖 Slither Analysis Report 🔎Slither report
# Slither report
_This comment was automatically generated by the GitHub Actions workflow._
THIS CHECKLIST IS NOT COMPLETE. Use
constable-statesImpact: Optimization
|
Changes to gas cost
🧾 Summary (5% most significant diffs)
Full diff report 👇
|
Pushed changes in this PR, will close #135 |
livingrockrises
merged commit b0f009a
into
fix/finding-18-withdraw-deposit-memory
Description:
execute()withEXECTYPE_TRYand eitherCALLTYPE_SINGLEorCALLTYPE_DELEGATECALL, no event is emitted if the try fails.CALLTYPE_BATCH, theTryExecuteUnsuccessfulevent is emitted for each failure.executeFromExecutor,TryDelegateCallUnsuccessfulandTryExecuteUnsuccessfulevents are emitted appropriately.