Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Unused function `Rescuable::initializeOwnership`

crisscs

Summary

Presence of an obsolete, unprotected function that initializes ownership - Rescuable::initializeOwnership.

The contract is an integral part of the functionality of the project as any vulnerabilities in it, can affect the derived classes

  • CapitalPool.sol

  • DeliveryPlace.sol

  • PreMarkets.sol

  • SystemConfig.sol

  • TokenManager.sol

Commit Hash: 04fd8634701697184a3f3a5558b41c109866e5f8

Repository URL: https://github.com/Cyfrin/2024-08-tadle/tree/main

Vulnerability Details

The owner of the Rescuable contract is initially set through the constructor, through the constructor of the Ownable contract

/// @notice Initializes the smart contract with the new implementation.
constructor() Ownable(_msgSender()) {}

The Rescuable contract contains the function Rescuable::initializeOwnership for initializing ownership.

The function Rescuable::initializeOwnership is not guarded by appropriate access control, enabling anyone to invoke it.

function initializeOwnership(address _newOwner) external {
if (owner() != address(0x0)) {
revert AlreadyInitialized();
}
​
_transferOwnership(_newOwner);
}

However, even if malicious actors attempt to invoke Rescuable::initializeOwnership, it will always revert with an AlreadyInitialized error, as the owner has been set in the constructor.

In summary, the function can be safely removed.

Impact

Malicious actors could attempt to transfer ownership by invoking the Rescuable::initializeOwnership function due to the missing access controls. However, given the owner initialization in the constructor, the function in its current form will revert, due to the following validation

if (owner() != address(0x0)) {
revert AlreadyInitialized();
}

Proof of Concept

  1. Add the following Rescuable.t.sol contract to the test directory

  2. Run forge test --match-contract Rescuable

  3. Observe the result

    Ran 4 tests for test/Rescuable.t.sol:RescuableTest
    [PASS] test_AttemptingToInitializeOwner_expectedRevertWithError() (gas: 13025)
    [PASS] test_givenMaliciousActorTryingToInitializeOwner_expectedInitializeOwnershipRevertsWithError() (gas: 13572)
    [PASS] test_givenMaliciousActorTryingToInitializeOwner_expectedOwnershipHasNotChanged() (gas: 14747)
    [PASS] test_initialOwnerIsDeployer() (gas: 7764)
    Suite result: ok. 4 passed; 0 failed; 0 skipped; finished in 4.29ms (372.46µs CPU time)
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;
​
import {Test} from "forge-std/Test.sol";
import {Rescuable} from "../src/utils/Rescuable.sol";
​
contract RescuableTest is Test {
Rescuable rescuable;
address maliciousActor = vm.addr(1);
address newPotentialOwner = vm.addr(2);
​
function setUp() public {
rescuable = new Rescuable();
}
​
function test_initialOwnerIsDeployer() public {
assertEq(rescuable.owner(), address(this));
}
​
function test_AttemptingToInitializeOwner_expectedRevertWithError() public {
vm.expectRevert(abi.encodeWithSelector(Rescuable.AlreadyInitialized.selector));
​
rescuable.initializeOwnership(newPotentialOwner);
}
​
function test_givenMaliciousActorTryingToInitializeOwner_expectedInitializeOwnershipRevertsWithError() public {
vm.expectRevert(abi.encodeWithSelector(Rescuable.AlreadyInitialized.selector));
​
vm.startPrank(maliciousActor);
rescuable.initializeOwnership(maliciousActor);
}
​
function test_givenMaliciousActorTryingToInitializeOwner_expectedOwnershipHasNotChanged() public {
address originalOwner = rescuable.owner();
​
vm.startPrank(maliciousActor);
try rescuable.initializeOwnership(maliciousActor) {
fail();
} catch {
assertEq(rescuable.owner(), originalOwner);
}
}
}

Tools Used

Recommendations

  1. Remove the** Rescuable::initializeOwnership **function since the constructor initializes the owner. This function should be removed to prevent confusion and potential security risks associated with its misuse.

  2. Ensure you have a robust test suite, covering access controls.

  3. Benefit from the usage of static analysis tools such as Slither and Aderyn to detect common vulnerabilities and bad practices.

  4. If there is a valid use case for dynamically changing ownership post-deployment, ensure the functionality is secured with appropriate access control modifiers like onlyOwner or similar, to prevent unauthorized use. Furthermore, consider using Ownable::transferOwnership

Updates

Lead Judging Commences

0xnevi Lead Judge
over 1 year ago
0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

[invalid] finding-Rescuable-initializeOwner-lack-access-control

Aside from `Rescuable.sol` being OOS, this is invalid based on codehawks guidelines regarding unprotected initializers. Additionally, this should be called concurrently when deploying a new proxy, but this submissions does not identify that particular issue of an uninitialized owner for proxy contracts

Support

FAQs

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

Give us feedback!