search

ubiquity / ubiquity-dollar

Ubiquity Dollar (uAD) smart contracts and user interface.
https://uad.ubq.fi
Apache License 2.0
31 stars 82 forks source link

Audit: fix low severity issues #874

Closed rndquu closed 3 months ago

rndquu commented 5 months ago

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

P.S. The list may be updated

0x4007 commented 5 months ago

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 [^1^]
  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 [^2^]

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.

[^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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

0x4007 commented 5 months ago

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

rndquu commented 5 months ago

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 commented 5 months ago

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 commented 4 months ago

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

rndquu commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/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 commented 4 months ago

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

gitcoindev commented 4 months ago

@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 https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/71#issuecomment-1900017138

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

https://github.com/ubiquity/ubiquity-dollar/blob/9f882429a844ca14118079c255368446bbafa8de/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L356 https://github.com/ubiquity/ubiquity-dollar/blob/9f882429a844ca14118079c255368446bbafa8de/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L428 https://github.com/ubiquity/ubiquity-dollar/blob/9f882429a844ca14118079c255368446bbafa8de/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L499

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 commented 4 months ago

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 commented 4 months ago

@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:

https://github.com/ubiquity/ubiquity-dollar/blob/9f882429a844ca14118079c255368446bbafa8de/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L356

https://github.com/ubiquity/ubiquity-dollar/blob/9f882429a844ca14118079c255368446bbafa8de/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L428

https://github.com/ubiquity/ubiquity-dollar/blob/9f882429a844ca14118079c255368446bbafa8de/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L499

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 https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/71 as invalid.

rndquu commented 4 months ago

Marking https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/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[bot] commented 3 months ago 
! No price label has been set. Skipping permit generation.