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

QA Report #7

Open
code423n4 opened this issue Feb 17, 2022 · 2 comments
Open

QA Report #7

code423n4 opened this issue Feb 17, 2022 · 2 comments
Labels
bug Something isn't working duplicate This issue or pull request already exists QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@code423n4
Copy link
Contributor

LOW

  1. Open TODO it seems that the logic is not finished and the logic might not be as expected.
  1. It seems that use an outdated version of openzeppelin

"@openzeppelin/contracts": "4.2.0",
"@openzeppelin/contracts-upgradeable": "4.3.2",

  1. The modification process of an owner is a delicate process, since the governance of our contract and therefore of the project may be at risk, for this reason it is recommended to adjust the owner’s modification logic, to a logic that allows to verify that the new owner is in fact valid and does exist.
    It's mandatory to create a logic of the owner’s modification where a new owner is proposed first, the owner accepts the proposal and, in this way, we make sure that there are no errors when writing the address of the new owner.
  1. The following contracts use Governable and can be paused, so a denial of service could happend if the keys are compromised or the issue number 3 happens.
  1. There are a lack of input checks around the contracts:
  1. The current lock logic does not contemplate ERC20 tokens with fee during the safeTransferFrom, therefore, the amount received by VUSD will be less than the expected.
    Some tokens may implement a fee during transfers, this is the case of USDT, even though the project has currently set it to 0. So, the transferFrom function would return 'true' despite receiving less than expected.
  1. Wrong logic around the method processWithdrawals

The logic is while (i < withdrawals.length && (i - start) <= maxWithdrawalProcesses) {

If maxWithdrawalProcesses=0 and start=0 then i=0. So (0-0)<=0 == true
But the expected maxWithdrawalProcesses is zero.

@code423n4 code423n4 added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax bug Something isn't working labels Feb 17, 2022
code423n4 added a commit that referenced this issue Feb 17, 2022
@atvanguard
Copy link
Collaborator

1 issue is a duplicate of #21 , disputing the rest.

@atvanguard atvanguard added the duplicate This issue or pull request already exists label Feb 26, 2022
@moose-code
Copy link
Collaborator

https://github.com/OpenZeppelin/openzeppelin-contracts/releases upgrading open zeppelin can be considered. Its good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working duplicate This issue or pull request already exists QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

4 participants