Staking Audit Fixes by ryzhak · Pull Request #999 · ubiquity/ubiquity-dollar

ryzhak · 2025-08-26T07:30:28Z

Resolves #998

In particular applies changes from #998 (comment).

Issue	Status
[H-01] Missing "mass pool update" on creating/updating a pool affects users' rewards	Fixed
[M-01] Staking and reward tokens overlapping with LibUbiquityPool's collateral skews calculations	Fixed
[M-02] Possible reentrancy	Fixed
[M-03] setStakingRewardToken causes DoS of stake/unstake methods	Acknowledged
[M-04] Staked tokens are stuck in the contract	Acknowledged
[M-05] Staking DoS if UBQ_MINTER_ROLE is revoked from the Diamond contract	Acknowledged
[M-06] User can get 0 rewards on high stake amounts	Fixed
[M-07] Dust reward tokens are stuck in the contract	Acknowledged
[M-08] Some "weird" ERC20 tokens are not supported	Acknowledged
[L-01] Unused imports	Fixed
[L-02] whenNotPaused modifier not used	Fixed
[L-03] Treasury griefing	Fixed
[L-04] governanceTreasuryDivider can't be set to 0	Fixed
[L-05] DoS when treasuryAddress is 0	Acknowledged

The Check For Diamond Storage Changes workflow is failing because of #992. Anyway it's safe to update the storage layout of the LibStaking since it's not deployed yet.

ubiquity-os · 2025-08-26T07:30:38Z

Caution

No labels are set.

ubiquity-os · 2025-08-26T07:31:10Z

Caution

No labels are set.

ubiquity-os · 2025-08-26T07:34:42Z

Caution

No labels are set.

ubiquity-os · 2025-08-26T07:36:02Z

Caution

No labels are set.

ubiquity-os · 2025-08-26T09:00:25Z

Caution

No labels are set.

ubiquity-os · 2025-08-26T15:34:15Z

Caution

No labels are set.

ubiquity-os · 2025-08-26T16:01:01Z

Caution

No labels are set.

ubiquity-os · 2025-08-26T16:02:17Z

Caution

No labels are set.

ubiquity-os · 2025-08-26T16:10:35Z

Caution

No labels are set.

ubiquity-os · 2025-08-26T16:11:25Z

Caution

No labels are set.

ubiquity-os · 2025-08-26T16:11:56Z

Caution

No labels are set.

ubiquity-os · 2025-08-26T16:12:54Z

Caution

No labels are set.

ubiquity-os · 2025-08-26T16:13:54Z

Caution

No labels are set.

ubiquity-os · 2025-08-26T16:18:08Z

Caution

No labels are set.

ubiquity-os · 2025-08-27T07:20:08Z

Caution

No labels are set.

ubiquity-os · 2025-08-27T07:43:20Z

Caution

No labels are set.

ubiquity-os · 2025-08-27T07:52:53Z

Caution

No labels are set.

ubiquity-os · 2025-08-27T07:59:54Z

Caution

No labels are set.

ryzhak · 2025-08-27T08:01:07Z

@0x4007 This PR is ready for review

0x4007 · 2025-08-27T08:01:36Z

Tip

Hello, world!

0x4007 · 2025-08-28T01:30:41Z

[!CAUTION]
No labels are set.

@gentlementlegen not sure why this sent so many consecutively this shouldn't happen (isBotEvent related?) also should be a warning not caution.

@ryzhak I'll review when I'm on my computer

packages/contracts/src/dollar/interfaces/IStaking.sol

packages/contracts/src/dollar/libraries/LibStaking.sol

0x4007

We should get more eyes on this.

Copilot

Pull Request Overview

This PR applies audit fixes for the staking functionality, addressing high and medium severity issues found during security review. The changes focus on improving reward calculations, preventing token overlap conflicts, and enhancing security through proper access controls.

Implements mass pool updates when creating/updating pools to ensure accurate reward calculations
Adds validation to prevent staking/reward tokens from overlapping with UbiquityPool collateral tokens
Adds reentrancy protection and pause functionality to critical staking operations

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`LibStaking.sol`	Core staking logic updates including mass pool updates, collateral overlap checks, and treasury divider validation
`StakingFacet.sol`	Added security modifiers (whenNotPaused, nonReentrant) and simplified function signatures
`IStaking.sol`	Updated interface to match simplified function signatures and added documentation
`StakingFacet.t.sol`	Updated tests to match new function signatures and added new test cases for collateral overlap validation
`StakingFacet.fuzz.t.sol`	Updated fuzz tests with new function signatures and improved parameter bounds
`AaveAmo.t.sol`	Fixed test to use valid function call instead of empty call

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

packages/contracts/src/dollar/libraries/LibStaking.sol

packages/contracts/test/diamond/facets/StakingFacet.t.sol

packages/contracts/test/fuzz/diamond/facets/StakingFacet.fuzz.t.sol

packages/contracts/test/diamond/facets/StakingFacet.t.sol

packages/contracts/src/dollar/facets/StakingFacet.sol

0x4007 · 2025-08-28T08:27:46Z

We should get more eyes on this.

zgo said they can take a look soon

zgorizzo69

good job fixing the issues from the audit
I would only add one small test and assess if feasible to make setStakingRewardToken revert if there are pending rewards

packages/contracts/test/diamond/facets/StakingFacet.t.sol

packages/contracts/src/dollar/facets/StakingFacet.sol

packages/contracts/src/dollar/libraries/LibStaking.sol

gentlementlegen · 2025-08-29T16:27:29Z

[!CAUTION]
No labels are set.

@gentlementlegen not sure why this sent so many consecutively this shouldn't happen (isBotEvent related?) also should be a warning not caution.

@ryzhak I'll review when I'm on my computer

This emanates from command-start-stop that tried to assign the user to the linked issue, unsuccessfully due to the labels lacking there. It reacts to pull-request.opened but also to pull_request.edited, so it tried to assign the user again on each edit.

0x4007 · 2025-08-30T03:30:57Z

@ryzhak I guess the failed CI doesn't matter?

ryzhak · 2025-09-01T14:42:24Z

@ryzhak I guess the failed CI doesn't matter?

Yes

The Check For Diamond Storage Changes workflow is failing because of #992. Anyway it's safe to update the storage layout of the LibStaking since it's not deployed yet.

0x4007 · 2025-09-02T16:34:11Z

@rndquu please resolve zgorizzo69's review comments- whether to implement or ignore is your decision

ryzhak · 2025-09-02T16:36:46Z

@rndquu please resolve zgorizzo69's review comments- whether to implement or ignore is your decision

All resolved, everything is kept "as is"

ryzhak added 2 commits August 26, 2025 10:25

ci: fix aave fork test

654794a

refactor: remove unused imports

f710920

feat(staking): add whenNotPaused modifier

4c8617d

feat(staking): add min amount for governance reward

0dcf7af

feat(staking): add ability to disable treasury rewards

e7fd49d

docs(staking): add notes regarding weird ERC20 tokens

1223434

feat(staking): add nonReentrant modifier

ccdba3e

refactor(staking): force mass pool updates

a38e0a5

feat(staking): assert stake/reward tokens not used as collateral

68083a3

ryzhak marked this pull request as ready for review August 27, 2025 08:00

ryzhak requested a review from rndquu as a code owner August 27, 2025 08:00

0x4007 reviewed Aug 28, 2025

View reviewed changes

packages/contracts/src/dollar/interfaces/IStaking.sol Show resolved Hide resolved

0x4007 reviewed Aug 28, 2025

View reviewed changes

packages/contracts/src/dollar/libraries/LibStaking.sol Show resolved Hide resolved

0x4007 reviewed Aug 28, 2025

View reviewed changes

packages/contracts/src/dollar/libraries/LibStaking.sol Show resolved Hide resolved

0x4007 approved these changes Aug 28, 2025

View reviewed changes

0x4007 requested a review from Copilot August 28, 2025 04:03

Copilot AI reviewed Aug 28, 2025

View reviewed changes

0x4007 reviewed Aug 28, 2025

View reviewed changes

packages/contracts/src/dollar/facets/StakingFacet.sol Show resolved Hide resolved

zgorizzo69 approved these changes Aug 29, 2025

View reviewed changes

0x4007 merged commit 7eb880f into ubiquity:development Sep 2, 2025
16 of 18 checks passed

ryzhak deleted the fix/audit-staking branch September 2, 2025 17:01

0x4007 mentioned this pull request Sep 3, 2025

Staking deploy script outdated #1001

Closed

ryzhak mentioned this pull request Sep 15, 2025

Fix staking migration #1004

Merged

Conversation

ryzhak commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ubiquity-os bot commented Aug 26, 2025

Uh oh!

ubiquity-os bot commented Aug 26, 2025

Uh oh!

ubiquity-os bot commented Aug 26, 2025

Uh oh!

ubiquity-os bot commented Aug 26, 2025

Uh oh!

ubiquity-os bot commented Aug 26, 2025

Uh oh!

ubiquity-os bot commented Aug 26, 2025

Uh oh!

ubiquity-os bot commented Aug 26, 2025

Uh oh!

ubiquity-os bot commented Aug 26, 2025

Uh oh!

ubiquity-os bot commented Aug 26, 2025

Uh oh!

ubiquity-os bot commented Aug 26, 2025

Uh oh!

ubiquity-os bot commented Aug 26, 2025

Uh oh!

ubiquity-os bot commented Aug 26, 2025

Uh oh!

ubiquity-os bot commented Aug 26, 2025

Uh oh!

ubiquity-os bot commented Aug 26, 2025

Uh oh!

ubiquity-os bot commented Aug 27, 2025

Uh oh!

ubiquity-os bot commented Aug 27, 2025

Uh oh!

ubiquity-os bot commented Aug 27, 2025

Uh oh!

ubiquity-os bot commented Aug 27, 2025

Uh oh!

ryzhak commented Aug 27, 2025

Uh oh!

0x4007 commented Aug 27, 2025

Uh oh!

0x4007 commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0x4007 left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0x4007 commented Aug 28, 2025

Uh oh!

zgorizzo69 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gentlementlegen commented Aug 29, 2025

Uh oh!

0x4007 commented Aug 30, 2025

Uh oh!

ryzhak commented Sep 1, 2025

Uh oh!

0x4007 commented Sep 2, 2025

ryzhak commented Aug 26, 2025 •

edited

Loading

0x4007 commented Aug 28, 2025 •

edited

Loading