Audit: fix low severity issues

[rndquu]

There are a bunch of low severity issues caught by auditors which are nice to fix. This is a low priority issue.

• chaduke - The owner might remove selector DiamondCutFacet.diamondCut() and then the protocol loses upgradablity. sherlock-audit/2023-12-ubiquity-judging#21 (removing diamondCut by accident)
• Krace - Users can still redeem their assets even after the collateral has been disabled. sherlock-audit/2023-12-ubiquity-judging#29 (disable collectRedemption() if collateral is disabled)
• Tendency - Deficient Implementation of redemptionDelayBlocks Requirement sherlock-audit/2023-12-ubiquity-judging#33 (more strict redemptionDelayBlocks)
• 0xpiken - ChainLink price feed address for collateral with index 0 might be updated mistakenly sherlock-audit/2023-12-ubiquity-judging#62 (set collateral chainlink price feed by index, admin may accidentally make the pool insolvent)
• rvierdiiev - When contract is paused, then users still can use it sherlock-audit/2023-12-ubiquity-judging#71 (missing whenNotPaused) - issue invalid due to different pause mechanism used. Marked as Sponsor Disputed, Won't Fix
• XDZIBEC - Reentrancy Vulnerability in EIP-2535 Diamond Standard Implementation sherlock-audit/2023-12-ubiquity-judging#119 (missing notReentrant modifier, not sure that it should be placed in the diamon's fallback, but worth to check all facet methods)
• r0ck3tz - Missing alignment of storage to 256 may result in excessive gas consumption sherlock-audit/2023-12-ubiquity-judging#121 (wrong implementation of ERC-7201, gas optimization)
• ilchovski - Admin can break the protocol logic irreversibly by adding the same collateral twice sherlock-audit/2023-12-ubiquity-judging#145 (don't allow admin to add 2 equal collateral addresses)
• popelev - Anyone can deposit to protocol tokens of other users without approve sherlock-audit/2023-12-ubiquity-judging#156 (transferFrom(receiver) in deprecated contract, we can't leave it this way even if the contract is not used)
• Bauchibred - Diamond.sol's fallback() seems to be erroneously implemented sherlock-audit/2023-12-ubiquity-judging#160 (pass msg.value in diamond's delegatecall, update: invalid)
• unforgiven - attacker can bypass function whitelist in Diamond pattern and call fallback() function because code doesn't check data.length>=4 sherlock-audit/2023-12-ubiquity-judging#183 (unauthorized fallback call with msg.data.length < 4 bytes)
• Varun_05 - Should not be allowed to mint dollar when collateral needed to mint dollar is equal to zero. sherlock-audit/2023-12-ubiquity-judging#207 (Dollars can be minted for 0 collateral for low decimal collaterals)
• 0xnirlin - Wrong number for blocks in a week sherlock-audit/2023-12-ubiquity-judging#230 (wrong number of blocks in a week) .

P.S. The list may be updated

[0x4007]

Just to clarify @gitcoindev @molecula451 because of:

P.S. The list may be updated

I think it makes sense to:

1. Reopen those issues on the Sherlock repository ¹
2. Open pull requests to @ubiquity/ubiquity-dollar and write something like Resolves https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/21 in the pull request body ²

This is to keep things in sync. After our adjustments are completed, we can mirror the issues back to the @ubiquity/ubiquity-dollar repository as a backup for auditing purposes (in case Sherlock decides to delete their repository.)

You guys should delete the mirrored issues on this repository now.

Footnotes

1. It is not clear to me why Sherlock closes them as complete. I suppose we technically can leave them closed as complete and still associate pull requests to them if it is a problem to reopen them. ↩

2. This automatically associates the issue with the pull request, which is useful for auditing in the future. ↩

[molecula451]

Yeah, i think it makes sense, all the mirroed issues were taken down, point to PR to resolve to the Resolves https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/X number issue

[rndquu]

Reopen those issues on the Sherlock repository

I don't think we should reopen them since those closed issues are considered invalid/low priority in terms of sherlock rules so the medium/high issues are only opened.

[gitcoindev]

Reopen those issues on the Sherlock repository

I don't think we should reopen them since those closed issues are considered invalid/low priority in terms of sherlock rules so the medium/high issues are only opened.

Cross-referencing them is a good practice. I am also not sure if Sherlock will keep this available forever, so we can mirror the issues back as @pavlovcik suggested, and skip reopening.

[molecula451]

By the sherklock repo it looks like they "archive" the whole judging repo, so i think we're fine with either way, mirroring now or mirroring later

[molecula451]

Recapping this one this issue posted here were closed specifically by the lead judge, i reopened one (to recheck) and he gave a reason to re-closed

[gitcoindev]

I am going to look into those issues and try to gradually fix them one by one starting tomorrow onward.

[0x4007]

I should be able to enable payouts directly in their repository once I make time to figure out the permit wallet situation.

[rndquu]

Update - this issue was marked invalid by Sherlock team

If some issue is marked as invalid by the Sherlock team it doesn't mean that we shouldn't fix it. Sherlock has pretty strict rules so only medium and high severity issues are considered valid hence the current "Audit: fix low severity issues" task was created specifically for low severity issues.

[gitcoindev]

Update - this issue was marked invalid by Sherlock team

If some issue is marked as invalid by the Sherlock team it doesn't mean that we shouldn't fix it. Sherlock has pretty strict rules so only medium and high severity issues are considered valid hence the current "Audit: fix low severity issues" task was created specifically for low severity issues.

All right, thank you for clarifying this, I update the description.

[molecula451]

rndqnuu the diamond it's not subceptible to reentraacy, i've seen others implementation live and they are working very well so sherlock-audit/2023-12-ubiquity-judging#119 not need to be overlook

[rndquu]

rndqnuu the diamond it's not subceptible to reentraacy, i've seen others implementation live and they are working very well so sherlock-audit/2023-12-ubiquity-judging#119 not need to be overlook

I don't think so, diamond does susceptible to reentrancy. Could you share some articles where it is stated that diamond is "reentrancy free"?

[molecula451]

I don't think so, diamond does susceptible to reentrancy. Could you share some articles where it is stated that diamond is "reentrancy free"?

in the fallback function where this watson pointed that it's prone to reentrant, doesn't look like, basically he's poiting that we lack the nonReetrant here on the fallback

https://etherscan.io/address/0x32400084c286cf3e17e7b677ea9583e60a000324#code

for instance there is this diamond with a couple$ without this modifier, looks highly debuggged, you'd jack that pot if you reentrant the fallback there

and to me this submission was from an automated tool without manually check after so it looks as false positive

no modifier on the fallback deployed on chain either: https://github.com/aavegotchi/aavegotchi-contracts/blob/7db9b732466f9b6e9b22d669f313f495750935b1/contracts/Aavegotchi/ForgeDiamond/ForgeDiamond.sol#L30

[rndquu]

I don't think so, diamond does susceptible to reentrancy. Could you share some articles where it is stated that diamond is "reentrancy free"?

in the fallback function where this watson pointed that it's prone to reentrant, doesn't look like, basically he's poiting that we lack the nonReetrant here on the fallback

https://etherscan.io/address/0x32400084c286cf3e17e7b677ea9583e60a000324#code

for instance there is this diamond with a couple$ without this modifier, looks highly debuggged, you'd jack that pot if you reentrant the fallback there

and to me this submission was from an automated tool without manually check after so it looks as false positive

no modifier on the fallback deployed on chain either: https://github.com/aavegotchi/aavegotchi-contracts/blob/7db9b732466f9b6e9b22d669f313f495750935b1/contracts/Aavegotchi/ForgeDiamond/ForgeDiamond.sol#L30

As a part of sherlock-audit/2023-12-ubiquity-judging#119 we don't need to add the nonReentrant modifier to the diamond's fallback but rather check all facets for this missing modifier (here, here, etc...)

[rndquu]

@gitcoindev Why did you mark sherlock-audit/2023-12-ubiquity-judging#71 as invalid? Don't we need the whenNotPaused() modifier here, here or here?

[gitcoindev]

@gitcoindev Why did you mark sherlock-audit/2023-12-ubiquity-judging#71 as invalid? Don't we need the whenNotPaused() modifier here, here or here?

Hi @rndquu ! I was guided by this comment sherlock-audit/2023-12-ubiquity-judging#71 (comment)

In the library we have already requires to protect against mint / redeem, for the examples that you provided:

ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol

Line 356 in 9f88242

require(

ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol

Line 428 in 9f88242

require(

ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol

Line 499 in 9f88242

require(

My understanding was that this mechanism shall be used but theoretically, if you think it makes sense, we could add whenNotPaused() on top as well, please let me know, as adding the modifier is straightforward.

[molecula451]

yeah this would be code redundancy, let's re-check tests on this requires and if they're good then no need for whenNotPaused() modifier, avoid redundancy

[rndquu]

@gitcoindev Why did you mark sherlock-audit/2023-12-ubiquity-judging#71 as invalid? Don't we need the whenNotPaused() modifier here, here or here?

Hi @rndquu ! I was guided by this comment sherlock-audit/2023-12-ubiquity-judging#71 (comment)

In the library we have already requires to protect against mint / redeem, for the examples that you provided:

ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol

Line 356 in 9f88242

require(

ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol

Line 428 in 9f88242

require(

ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol

Line 499 in 9f88242

require(

My understanding was that this mechanism shall be used but theoretically, if you think it makes sense, we could add whenNotPaused() on top as well, please let me know, as adding the modifier is straightforward.

Yes, you're right, I've totally forgotten that https://github.com/ubiquity/ubiquity-dollar/blob/development/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol has its own pause mechanic. It's ok then to mark sherlock-audit/2023-12-ubiquity-judging#71 as invalid.

[rndquu]

Marking sherlock-audit/2023-12-ubiquity-judging#160 as invalid because:

1. Yul's delegatecall() doesn't have msg.value in params (check the docs and related SO question here)
2. I've double checked our Diamond implementation, all payable methods are working fine (i.e. accept ETH if you send it)

[ubiquibot]

! No price label has been set. Skipping permit generation.