code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/counter-example/v2/CounterV2.sol#L32 https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/counter-example/v2/CounterV2.sol#L37

Vulnerability details

Impact

Initialize function in CounterV2.sol can be invoked multiple times from the implementation contract. This means a compromised implementation can reinitialize the contract above.

Usually in Upgradeable contract, an initialize function is protected by the modifier initializer to make sure the contract can only be initialized once.

Proof of Concept

The implementation contract is compromised,
The attacker reinitialize the CounterV2 contract

    function initialize(
        IDAO _dao,
        MultiplyHelper _multiplyHelper,
        uint256 _count,
        uint256 _newVariable
    ) external reinitializer(2) {
        __PluginUUPSUpgradeable_init(_dao);

        count = _count;

        // Since this is V2 version, and some daos might want to install this right away
        // without installing CountV1, dev decides to also include setting newVariable
        newVariable = _newVariable;

        multiplyHelper = _multiplyHelper;
    }

Tools Used

VS Code

Recommended Mitigation Steps

We recommend the project use the modifier initializer to protect the initialize function from being reinitiated

    function initialize(
        IDAO _dao,
        MultiplyHelper _multiplyHelper,
        uint256 _count,
        uint256 _newVariable
    ) external initializer {

0xean commented 1 year ago

The design of the re-initializer is already discussed in the docs from the sponsor.

c4-judge commented 1 year ago

0xean marked the issue as unsatisfactory: Insufficient quality

code-423n4 / 2023-03-aragon-findings