Open hats-bug-reporter[bot] opened 5 months ago
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.
@krzysztofziobro Disagree with the decision as this contract is in scope and the code logic contains bugs.
Valid submission - minor
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
:This function let owner to specify
address new_address
and the contract will callsetCompleted
in that address and set the value tolast_completed_migration
. However, the value oflast_completed_migration
inupgraded
address will not be able to set properly because ofrestricted
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 thedeployer/owner
of the new migration contract. As a result, thelast_completed_migration
value in new migration contract cannot be set properly by callingMigrations::upgrade
function.Proof of concept is given below for more details.
Attachments
NA
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:import {Test, console2 as console} from "forge-std/Test.sol"; import {Migrations} from "../src/Migrations.sol";
contract MigrationsTest is Test { Migrations public migration;
}
NA