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

Incorrect Return Value in mul_mod Function #10

Open
howlbot-integration bot opened this issue Nov 4, 2024 · 6 comments
Open

Incorrect Return Value in mul_mod Function #10

howlbot-integration bot opened this issue Nov 4, 2024 · 6 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-b primary issue Highest quality submission among a set of duplicates Q-03 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-10-superposition/blob/7ad51104a8514d46e5c3d756264564426f2927fe/pkg/seawater/src/maths/full_math.rs#L28

Vulnerability details

The mul_mod function is intended to calculate the result of
a×bmodc, but due to a logical issue, it returns the modified modulus (denoted as c) instead of the result of the modulo operation. This leads to incorrect outcomes when using this function, which can result in unintended behavior, especially in critical financial or cryptographic operations where correct modular arithmetic is essential.

The issue with the return value in the mul_mod function arises because the function is intended to compute
a×bmodc — i.e., it should return the result of multiplying a and b, and then taking the remainder when that product is divided by modulus (denoted as c).
However, the current implementation mistakenly returns the modified modulus itself, rather than the remainder
Why is this a problem?
In modular arithmetic, you typically want to compute a result like this:
result=(a×b)%c This means you:

Multiply a and b together to get a large number (the "product").
Divide the product by c (modulus), and return the remainder from that division as the result.

Instead, the function currently:
Multiplies a and b correctly, storing the product in a temporary variable.
Modifies the modulus variable by performing division (which likely updates its internal representation).
Returns the modified modulus, which is not the result of
a×bmodc.

Example of Correct Logic:
Consider a = 10, b = 20, and c = 7.

200÷7=28 with a remainder of 4.
The function should return 4 because
200%7=4.

Instead, the current function is returning c (or modulus), which is 7, not the remainder 4.

The function needs to return the remainder of the division, not the modified modulus.

Assessed type

Other

@howlbot-integration howlbot-integration bot added 3 (High Risk) Assets can be stolen/lost/compromised directly 🤖_primary AI based primary recommendation bug Something isn't working primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels Nov 4, 2024
howlbot-integration bot added a commit that referenced this issue Nov 4, 2024
@af-afk
Copy link

af-afk commented Nov 8, 2024

Thankfully this isn't in use. We're going to rip this function out.

@af-afk af-afk added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Nov 8, 2024
@af-afk
Copy link

af-afk commented Nov 11, 2024

Is it fair to say this is a high issue if we're not using this code?

@alex-ppg
Copy link

The Warden has identified a severe flaw in the full_math::mul_mod function as it presently yields the modulo operand instead of the result of the calculation.

As the function remains unused in the codebase, it has a zero likelihood of manifesting in the compiled code of the project and thus merits a QA severity rating.

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Nov 13, 2024
@c4-judge
Copy link

alex-ppg changed the severity to QA (Quality Assurance)

@c4-judge
Copy link

alex-ppg marked the issue as grade-b

@af-afk
Copy link

af-afk commented Nov 25, 2024

@C4-Staff C4-Staff added the Q-03 label Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-b primary issue Highest quality submission among a set of duplicates Q-03 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

4 participants