QA Report

[code423n4]

See the markdown file with the details of this report here.

[c4-judge]

0xean marked the issue as grade-b

[novaknole20]

1. We have to neglect import statement symboling. We cover it mostly for our contracts. Doesn't bring any gas or security advantage.
2. The createVersion function should validate that _release >= latestRelease, else subtraction in line 143 will overflow. - if release 4 exists, dev might be wanting to push new build to release 1. the check _release >= latestRelease would not make it possible.
3. Validate initialOwner address in PluginRepoFactory.createPluginRepo - well, validating it with address(0) wouldn't bring any advantage. address(0) would mostly happen if dev is kind of malicious and if so, even with the check, he would try address(1) and still achieve mostly the same thing.
4. Validate _maintainer address in PluginRepoFactory.createPluginRepoWithFirstVersion - the same as (3)
5. The applyUpdate function in PluginSetupProcessor should validate that the current version of the plugin - we already do since the current version is included in the appliedSetupId generation.
6. After creating the MerkleMinter instance, the createToken function present in the TokenFactory contract should also grant the DAO the CHANGE_DISTRIBUTOR_PERMISSION_ID permission. - We might have forgotten it, but not security or gas suggestion. In the end, they can always grant it through proposal. This is kind of permission that would never be required in 99.999% cases, so more gas cost when installing plugin is pointless.
7. CounterV2 setup example is missing "newVariable" in installation initializer - Correct, but I believe this is not real contract and deployed. So no security or gas problem. Let's discuss this in the end.

[c4-sponsor]

novaknole20 marked the issue as sponsor disputed

[c4-sponsor]

novaknole20 requested judge review

[0xean]

Wardens QA reports include #180, #174, #173 believe this award to be reflective of the overall report with these included.

[novaknole20]

@0xean all 2 of your mentioned issues are from our point of view not applicable:

180: As mentioned in the comment there. This is how it should work

173: Actions are verified by the DAO members. Also we want to interact with EOA wallets, thus a verification if the target address has code is not necessary

[c4-judge]

0xean marked the issue as grade-c

[romeroadrian]

The createVersion function should validate that _release >= latestRelease, else subtraction in line 143 will overflow. - if release 4 exists, dev might be wanting to push new build to release 1. the check _release >= latestRelease would not make it possible.

In the provided example, if release 4 exists (and this is the latest release) and dev want to push to release 1, then the subtraction in line 143 will overflow since it will calculate the expression 1 - 4 in unsigned arithmetic (both variables are uint8). This means that the pointed issue is a bit worse, since it won't let users push builds to previous releases.

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepo.sol#L143-L145

if (_release - latestRelease > 1) {
    revert InvalidReleaseIncrement({latestRelease: latestRelease, newRelease: _release});
}

If the intention is to validate new releases are not incremented by more than one, but allow pushes to previous releases then this check should make more sense:

if (_release > latestRelease && _release - latestRelease > 1) {
    revert InvalidReleaseIncrement({latestRelease: latestRelease, newRelease: _release});
}

---

Validate initialOwner address in PluginRepoFactory.createPluginRepo - well, validating it with address(0) wouldn't bring any advantage. address(0) would mostly happen if dev is kind of malicious and if so, even with the check, he would try address(1) and still achieve mostly the same thing.

Validate _maintainer address in PluginRepoFactory.createPluginRepoWithFirstVersion - the same as (3)

I understand the comment, but usually the recommendation is to check against address(0) as this represents the empty/default value.

---

Regarding #180 , given the notes and the recommendations, even though the solution or workaround is easy, the issue was raised as the specs clearly don't document this (https://devs.aragon.org/docs/osx/how-it-works/framework/plugin-management/plugin-setup/#setup-application).

---

For #173, note that contracts may also selfdestruct while the proposal is being reviewed/voted and the actions are still pending. I think is still a valid issue, even if it is categorized as low.

[0xean]

@romeroadrian - appreciate the response and that your report is clearly not automated, which is refreshing. I think you are right on the cusp of a "B" grade, but not quite there. Between the points already raised by the sponsor and item(s) that are duplicates of known issues I am going to leave the grade as is.

[0xean]

re

In the provided example, if release 4 exists (and this is the latest release) and dev want to push to release 1,

I don't believe this is the intended functionality. You are suggesting support for multiple releases simultaneously which I cannot find reference to in the documentation. @novaknole20 can you confirm that this is not the intended functionality?

[romeroadrian]

@romeroadrian - appreciate the response and that your report is clearly not automated, which is refreshing. I think you are right on the cusp of a "B" grade, but not quite there. Between the points already raised by the sponsor and item(s) that are duplicates of known issues I am going to leave the grade as is.

Note that the reported instances of the address(0) check are not in the report by picodes (if this is what you meant by duplicates of known issues).

I also think that this comment from the sponsor is not accurate in relation to the reported issue:

The applyUpdate function in PluginSetupProcessor should validate that the current version of the plugin - we already do since the current version is included in the appliedSetupId generation.

The applyUpdate function takes a plugin setup reference (_params.pluginSetupRef) of the new version, this is what is included in the hash of preparedSetupId, the current version is not included in this hash (see lines 446 and 475). The current version is only validated when prepareUpdate is called, and not at execution time.