Biconomy: Nexus

Summary

The _handleSingleExecutionAndReturnData function doesnt consistently handle execution success between different execution types (DEFAULT and TRY).

Vulnerability Details

In the _handleSingleExecutionAndReturnData function: https://github.com/Cyfrin/2024-07-biconomy/blob/9590f25cd63f7ad2c54feb618036984774f3879d/contracts/base/ExecutionHelper.sol#L170-L183

As seen below, for EXECTYPE_DEFAULT:

The function assumes success if _execute doesn't revert, without explicitly checking or returning a success status.

For EXECTYPE_TRY:

The function properly checks the success flag and emits an event for unsuccessful executions.

This means that for EXECTYPE_DEFAULT, there's no way to distinguish between a successful execution that returns data and a failed execution that returns an error message. Both cases are treated as successful.

Impact

There could be silent failures in the sense that failed executions in EXECTYPE_DEFAULT mode might go unnoticed. There could be scenarios where a contract execution doesn't revert but still fails in some way, and this wouldn't be captured or reported by the current implementation.

Tools Used

Manual review

Recommendations

Modify the _execute function to return a boolean success flag along with the result, similar to _tryExecute and then update the _handleSingleExecutionAndReturnData function to use the success flag consistently.