Wrong implementation in Migrations::upgrade causes this function doesn't work as expected.

[hats-bug-reporter]

Github username: @erictee2802
Twitter username: 0xEricTee
Submission hash (on-chain): 0x662fb1a311f1f8553e9b0006f18efbda9693dac7466b1b3584d37b02ca10748d
Severity: low

Description:
Description

Wrong implementation in Migrations::upgrade causes this function doesn't work as expected.

Attack Scenario

In Migrations::upgrade:

   function upgrade(address new_address) public restricted {
        Migrations upgraded = Migrations(new_address);
        upgraded.setCompleted(last_completed_migration);
    }

This function let owner to specify address new_address and the contract will call setCompleted in that address and set the value to last_completed_migration. However, the value of last_completed_migration in upgraded address will not be able to set properly because of restricted modifier.

The issue arises as the old migration contract trying to call setCompleted function on new migration contract however the old migration contract is not the deployer/owner of the new migration contract. As a result, the last_completed_migration value in new migration contract cannot be set properly by calling Migrations::upgrade function.

Proof of concept is given below for more details.

Attachments

NA

1. Proof of Concept (PoC) File

• Install foundry.

• Create an empty folder named aleph_zero.

• Inside this folder, execute forge init.

• Add the Migrations.sol contract to the /src folder.

• Add the Migrations.t.sol contract inside /test folder with the following content:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console2 as console} from "forge-std/Test.sol";
import {Migrations} from "../src/Migrations.sol";

contract MigrationsTest is Test {
    Migrations public migration;

    function setUp() public {
        migration = new Migrations();
    }

    function test_UpgradeNotWorking() external {
        migration.setCompleted(1);
        require(migration.last_completed_migration() == uint(1));

        // Owner decides to migrate to new migration contract.

        Migrations migration2 = new Migrations();
        migration.upgrade(address(migration2));

        //Expect the last_completed_migration of migration2 to be 1 but in reality the value is 0.
        console.log("Expected last_completed_migration of Migration2: 1");
        console.log("Real last_completed_migration of Migration2: ", migration2.last_completed_migration());



    }
}

• Launch the foundry test with command: forge test --match-test test_UpgradeNotWorking -vv:

Foundry Result:

Running 1 test for test/Migration.t.sol:MigrationsTest
[PASS] test_UpgradeNotWorking() (gas: 178266)
Logs:
  Expected last_completed_migration of Migration2: 1
  Real last_completed_migration of Migration2:  0

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 10.93ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

2. Revised Code File (Optional)

NA

[fbielejec]

Valid submission & a reproducible POC, although for a bit different reasons than mentioned - Migrations.sol and it's ink! counterpart should not be upgradeable at all - they are merely keeping an on-chain counter, so there is no logic to upgrade. Recommendation should advocate removing upgrade tx from the sol contract altogether and save some gas on deployment.

[0xEricTee]

@krzysztofziobro Disagree with the decision as this contract is in scope and the code logic contains bugs.

[krzysztofziobro]

Valid submission - minor