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

Fix Oracle PostPersist #2074

Merged
merged 24 commits into from
Nov 27, 2020
Merged

Fix Oracle PostPersist #2074

merged 24 commits into from
Nov 27, 2020

Conversation

shargon
Copy link
Member

@shargon shargon commented Nov 19, 2020

When processing an oracle’s response transaction, it is not verified that the transaction is successful which allows a group of oracle’s nodes to sign a transaction that will throw an exception in the PostPersist method, causing a Denial of Service.

Found by @Red4Sec

Copy link
Contributor

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

Isn't it solved by

OracleRequest request = NativeContract.Oracle.GetRequest(snapshot, Id);
if (request is null) return false;
check? The only problem could be multiple transactions for the same ID.

src/neo/SmartContract/Native/Oracle/OracleContract.cs Outdated Show resolved Hide resolved
Tommo-L
Tommo-L previously approved these changes Nov 19, 2020
if (oracle != null &&
(!oracleRequests.TryGetValue(oracle.Id, out var hash) || hash != tx.Hash))
{
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, this can be replaced with generic "Conflicts" mechanism (#1991) that would also allow for proper transaction to have higher priority over fallback.

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

Regarding the description of checking if the transaction was sucesfull you removed that part, right?

As it is right now it is only improving the caching mechanism for avoiding duplicate txs?

@Tommo-L
Copy link
Contributor

Tommo-L commented Nov 25, 2020

UT failed

/// Store oracle responses
/// </summary>
private readonly Dictionary<ulong, UInt256> oracleResponses = new Dictionary<ulong, UInt256>();

public void AddTransaction(Transaction tx)
Copy link
Member Author

@shargon shargon Nov 26, 2020

Choose a reason for hiding this comment

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

Why don't remove CheckTransaction and Change Add to bool TryAdd (thread-safe)?

Copy link
Member

Choose a reason for hiding this comment

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

Because CheckTransaction is called in Transaction.Verify().

@Tommo-L
Copy link
Contributor

Tommo-L commented Nov 27, 2020

Merge?

@erikzhang erikzhang merged commit b6ebbbe into neo-project:master Nov 27, 2020
@shargon shargon deleted the fix-oracle branch November 27, 2020 09:31
ShawnYun pushed a commit to ShawnYun/neo that referenced this pull request Jan 8, 2021
cloud8little pushed a commit to cloud8little/neo that referenced this pull request Jan 24, 2021
ixje added a commit to CityOfZion/neo-mamba that referenced this pull request Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants