search

code-423n4 / 2022-02-skale-findings

0 stars 0 forks source link

QA Report #29

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

  1. Used an outdated solidity compiler with known bugs.

  2. It seems that was used an outdated version of openzeppelin, the last version is 4.5.0

    "@openzeppelin/contracts-upgradeable": "^4.3.2"

  3. DepositBox whitelist is enabled by default, so it's more a black list than a whitelist.

  4. There are a lack of input checks around the contracts:

  5. The logic applied to emit an event of the change of a variable, does not check that the change is to the same value as the current one, it should be omitted to launch a change event if the defined value is the same, otherwise, the dApps could have wrong logics

  6. Is not possible to change the owner, this is a very bad practice, the private key could be leaked, or the admin could be revoked.

DimaStebaev commented 2 years ago
  1. Initialization is performed automatically by a script
  2. It's just an example and should not be used in production
GalloDaSballo commented 2 years ago

Used an outdated solidity compiler with known bugs.

Agree because of actual evidence, would recommend the sponsor to update to 0.8.9+ (0.8.11 seems to be fairly used)

It seems that was used an outdated version of openzeppelin, the last version is 4.5.0

I think the finding has validity but I wouldn't stress too much on that, would recommend diffind the codebases for any patches

Depositbox whitelist is enabled by default, so it's more a black list than a whitelist.

Don't think it's a vulnerability

There are a lack of input checks around the contracts:

Finding is valid

The logic applied to emit an event of the change of a variable, does not check that the change is to the same value as the current one, it should be omitted to launch a change event if the defined value is the same, otherwise, the dApps could have wrong logics

Agree but non-critical / informational

Is not possible to change the owner, this is a very bad practice, the private key could be leaked, or the admin could be revoked.

Appreciate the finding, but believe this is a testing contract