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: assert that the binary decomposition of a variable is less than the modulus #835

Merged
merged 18 commits into from
Sep 19, 2023

Conversation

ivokub
Copy link
Collaborator

@ivokub ivokub commented Sep 15, 2023

The following issue was reported by Marcin Kostrzewa @ Reilabs (@kustosz). We really appreciate the detailed report!

Description

When doing a binary decomposition of a value, we compute the bit values inside the hint and then assert that the linear combination of the bits adds up to the initial value:

value = \sum_{i} 2^i bit_i

However, as the sum is computed inside the circuit, then for many values there actually exists two valid decompositions: value and value+Fr where Fr is the modulus of the scalar field. Depending on the applications, the non-uniqueness either may or may not be an issue, but this problem affects comparison and inequality assertion where we had this assumption. This may lead to non-sound comparisons if a malicious prover replaces the binary decomposition hint function.

Fixes #836

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How has this been tested?

  • Added test cases for api.AssertIsLessEq, api.Cmp which failed before bugfixes and succeeded after.
  • Added test cases for std/math/cmp

How has this been benchmarked?

The fixes have impact on AssertIsLessEq and Cmp. Recommendation is to use std/math/cmp if the bound is known. We are also looking at more efficient comparison methods (see #831).

  • Updated statistics.

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I did not modify files generated from templates
  • golangci-lint does not output errors locally
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@ivokub ivokub added bug Something isn't working P1: High Issue priority: high labels Sep 15, 2023
@ivokub ivokub self-assigned this Sep 15, 2023
@github-actions
Copy link

Summary

✅ Passed: 5709
❌ Failed: 0
🚧 Skipped: 21

🚧 Skipped

  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bls12-381/mpcsetup)
  • TestSetupCircuit (github.com/consensys/gnark/backend/groth16/bls12-381/mpcsetup)
  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bls12-377/mpcsetup)
  • TestSetupCircuit (github.com/consensys/gnark/backend/groth16/bls12-377/mpcsetup)
  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bls24-315/mpcsetup)
  • TestSetupCircuit (github.com/consensys/gnark/backend/groth16/bls24-315/mpcsetup)
  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bls24-317/mpcsetup)
  • TestSetupCircuit (github.com/consensys/gnark/backend/groth16/bls24-317/mpcsetup)
  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bn254/mpcsetup)
  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bw6-633/mpcsetup)
  • TestSetupCircuit (github.com/consensys/gnark/backend/groth16/bw6-633/mpcsetup)
  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bw6-761/mpcsetup)
  • TestSetupCircuit (github.com/consensys/gnark/backend/groth16/bw6-761/mpcsetup)
  • TestCircuitInclusionProof (github.com/consensys/gnark/examples/rollup)
  • TestCircuitUpdateAccount (github.com/consensys/gnark/examples/rollup)
  • TestCircuitFull (github.com/consensys/gnark/examples/rollup)
  • TestFrobeniusFp12 (github.com/consensys/gnark/std/algebra/native/fields_bls12377)
  • TestMulByNonResidueFp2 (github.com/consensys/gnark/std/algebra/native/fields_bls12377)
  • TestMulByFp2Fp6 (github.com/consensys/gnark/std/algebra/native/fields_bls12377)
  • TestIssue348UnconstrainedLimbs (github.com/consensys/gnark/std/math/emulated)
  • TestSolverConsistency (github.com/consensys/gnark/test)

Copy link
Collaborator

@gbotrel gbotrel left a comment

Choose a reason for hiding this comment

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

not sure about the IsTestEngine() part but other than that the PR looks good to me! 👍

@ivokub
Copy link
Collaborator Author

ivokub commented Sep 17, 2023

not sure about the IsTestEngine() part but other than that the PR looks good to me! 👍

Yeah, it was ugly. I'll see if I can do without.

@ivokub
Copy link
Collaborator Author

ivokub commented Sep 19, 2023

Removed IsTestEngine and implemented the missing method in the test engine instead.

@github-actions
Copy link

Summary

✅ Passed: 5709
❌ Failed: 0
🚧 Skipped: 21

🚧 Skipped

  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bls12-377/mpcsetup)
  • TestSetupCircuit (github.com/consensys/gnark/backend/groth16/bls12-377/mpcsetup)
  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bls12-381/mpcsetup)
  • TestSetupCircuit (github.com/consensys/gnark/backend/groth16/bls12-381/mpcsetup)
  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bls24-315/mpcsetup)
  • TestSetupCircuit (github.com/consensys/gnark/backend/groth16/bls24-315/mpcsetup)
  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bls24-317/mpcsetup)
  • TestSetupCircuit (github.com/consensys/gnark/backend/groth16/bls24-317/mpcsetup)
  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bn254/mpcsetup)
  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bw6-633/mpcsetup)
  • TestSetupCircuit (github.com/consensys/gnark/backend/groth16/bw6-633/mpcsetup)
  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bw6-761/mpcsetup)
  • TestSetupCircuit (github.com/consensys/gnark/backend/groth16/bw6-761/mpcsetup)
  • TestCircuitInclusionProof (github.com/consensys/gnark/examples/rollup)
  • TestCircuitUpdateAccount (github.com/consensys/gnark/examples/rollup)
  • TestCircuitFull (github.com/consensys/gnark/examples/rollup)
  • TestFrobeniusFp12 (github.com/consensys/gnark/std/algebra/native/fields_bls12377)
  • TestMulByNonResidueFp2 (github.com/consensys/gnark/std/algebra/native/fields_bls12377)
  • TestMulByFp2Fp6 (github.com/consensys/gnark/std/algebra/native/fields_bls12377)
  • TestIssue348UnconstrainedLimbs (github.com/consensys/gnark/std/math/emulated)
  • TestSolverConsistency (github.com/consensys/gnark/test)

@ivokub ivokub merged commit 59a4087 into master Sep 19, 2023
6 checks passed
@ivokub ivokub deleted the fix/cmp-reducecheck branch September 19, 2023 08:26
@samuelmonday699
Copy link

  • [ ]
    Duplicate of #

@Jircs1
Copy link

Jircs1 commented Jan 27, 2024

#835 (comment)

@Foxsenrubas
Copy link

fix/cmp-reducecheck

@Foxsenrubas
Copy link

Foxsenrubas commented May 1, 2024

О следующей проблеме сообщил Марцин Костшева @ Reilabs (@kustosz). Мы очень ценим подробный отчет!

Описание

При двоичном разложении значения мы вычисляем значения битов внутри подсказки, а затем утверждаем, что линейная комбинация битов в сумме дает начальное значение:

value = \sum_{i} 2^i bit_i

Однако, поскольку сумма вычисляется внутри схемы, то для многих значений фактически существует два допустимых разложения: valueи value+Frгде Fr– модуль скалярного поля. В зависимости от приложений неединственность может быть проблемой, а может и не быть, но эта проблема влияет на сравнение и утверждение неравенства там, где у нас было это предположение. Это может привести к необоснованным сравнениям, если злоумышленник заменит функцию подсказки двоичного разложения.

Исправления №836

Тип изменения

  • Исправление ошибки (некритическое изменение, устраняющее проблему)

Как это было проверено?

  • Добавлены тестовые примеры для api.AssertIsLessEq, api.Cmpкоторые не прошли до исправления ошибок и завершились успешно после.
  • Добавлены тестовые примеры дляstd/math/cmp

Как это оценивалось?

Исправления влияют на AssertIsLessEq и Cmp. Рекомендуется использовать std/math/cmp, если граница известна. Мы также ищем более эффективные методы сравнения (см. #831 ).

  • Обновленная статистика.

Контрольный список:

  • Я выполнил самопроверку своего кода
  • Я прокомментировал свой код, особенно в труднопонятных местах.
  • Я внес соответствующие изменения в документацию
  • Я добавил тесты, которые доказывают, что мое исправление эффективно или моя функция работает.
  • Я не изменял файлы, созданные на основе шаблонов.
  • golangci-lintне выводит ошибки локально
  • Новые и существующие модульные тесты проходят локально с моими изменениями.
  • Любые зависимые изменения были объединены и опубликованы в последующих модулях.
  • value = \sum_{i} 2^i bit_i

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P1: High Issue priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: binary decomposition of a variable is not asserted to be less than the modulus
5 participants