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

πŸ‘·πŸ»β€β™‚οΈ EIP7412 Fix #2

Merged
merged 17 commits into from
Nov 15, 2023
Merged

Conversation

JaredBorders
Copy link
Contributor

Summary

Use the EIP7412 selector for fulfillOracleQuery and not the IERC7412 selector.

Concerns

Given

function fulfillOracleQuery(
    address EIP7412Implementer,
    bytes calldata signedOffchainData
) external payable {
    IERC7412(EIP7412Implementer).fulfillOracleQuery(signedOffchainData);
}

With EIP7412Implementer being variable, does this present a risk?

Copy link

@tommyrharper tommyrharper left a comment

Choose a reason for hiding this comment

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

As discussed on call, we decided to not allow users to execute fulfillOracleQuery with an arbitrary EIP7412Implementer address to avoid the possibility of a malicious actor who gains access to a biconomy EOA wallet private key being able to make a reentrant attack on Smart Margin V3 - despite the fact that Smart Margin V3 is in theory reentrant secure.

We decided this based on the principle of erring on the side of paranoia in security related decisions.

Instead we can define the EIP7412Implementer address as immutable at the smart contract engine level, and provide a different address for the different engines on different chains.

This removes the potential (though extremely unlikely) reentrant attack vector.

Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (389b804) 47.91% compared to head (0b6bc82) 50.00%.

Files Patch % Lines
src/kwenta/smv3/EIP7412.sol 0.00% 1 Missing ⚠️
test/SMv3SessionValidationModule.t.sol 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main       #2      +/-   ##
==========================================
+ Coverage   47.91%   50.00%   +2.08%     
==========================================
  Files           9       10       +1     
  Lines         192      186       -6     
  Branches       24       20       -4     
==========================================
+ Hits           92       93       +1     
+ Misses         96       89       -7     
  Partials        4        4              

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

Copy link

@tommyrharper tommyrharper left a comment

Choose a reason for hiding this comment

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

LGTM πŸ‘ - as long as we are happy that SMv3 is reentrant secure

@JaredBorders JaredBorders merged commit 0be4e7e into main Nov 15, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants