`Migrations.upgrade()` should be removed from contract

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x02fa6a45eaac969c9c44f512189e58353055fe9bb716add902f576091b333023 Severity: minor

Description: Description\

Referencing the latest issue comments from Protocol team,

1) https://github.com/hats-finance/Most--Aleph-Zero-Bridge-0xab7c1d45ae21e7133574746b2985c58e0ae2e61d/issues/40#issuecomment-2031978289

If anything the upgrade tx should be removed from the Solidity version of the contract as unnecessary.

2) https://github.com/hats-finance/Most--Aleph-Zero-Bridge-0xab7c1d45ae21e7133574746b2985c58e0ae2e61d/issues/3#issuecomment-2032014382

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.

Based on the above comments, upgrade() function should be removed.

Further, the Migration contract is not consistent with ink! migration contract as there is no upgrade()function inink!` migration contract. This issue further confirms, there is no need of upgrade() function in contract.

Removing the upgrade() function from Migration.sol will reduce the byte code which will also reduce the contract size and most importantly result in gas optimization.

Recommendations\

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.20;

contract Migrations {
    address public immutable owner;
    uint256 public last_completed_migration;

    modifier restricted() {
        if (msg.sender == owner) _;
    }

    constructor() {
        owner = msg.sender;
    }

    function setCompleted(uint256 completed) public restricted {
        last_completed_migration = completed;
    }

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

[0xRizwan]

Another issue which is not highlighted:

When the upgrade() will be called by owner of contract, it will always revert as the setCompleted() can only be called by owner of contract but the msg.sender in this case will be Migration.sol contract.

In short, the flow will look as:

Owner calls ---> upgrade() function ---> upgrade() further calls setCompleted() ---> setCompleted() is being called here from Migration contract as msg.sender ---> But setCompleted() can be accessed by contract owner ---> Therefore, it will revert and upgradation can not be acheived.

Therefore, its better to remove upgrade() from Migration.sol

[krzysztofziobro]

Invalid submission - no PoC

[0xRizwan]

Another issue which is not highlighted:

When the upgrade() will be called by owner of contract, it will always revert as the setCompleted() can only be called by owner of contract but the msg.sender in this case will be Migration.sol contract.

In short, the flow will look as:

Owner calls ---> upgrade() function ---> upgrade() further calls setCompleted() ---> setCompleted() is being called here from Migration contract as msg.sender ---> But setCompleted() can be accessed by contract owner ---> Therefore, it will revert and upgradation can not be acheived.

Therefore, its better to remove upgrade() from Migration.sol

@krzysztofziobro I explained the issue in this comment and above description. Can you please mention what you didn't understand about this issue. The issue is submitted as Minor severity so mentioned the theortical description in issue flow in this comment.