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

Missing zero address check when creating TadleFactory

crisscs

Summary

The missing zero address check in the constructor of TadleFactory when setting guardian could result in rendering it unusable and unable to deploy contracts.

Commit Hash: 04fd8634701697184a3f3a5558b41c109866e5f8

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

Vulnerability Details

The constructor of TadleFactory receives an address for the guardian, however, no zero address check is performed.

constructor(address _guardian) {
guardian = _guardian;
}

If address(0x0) is passed to the constructor, it would prevent from further calling the TadleFactory::deployUpgradeableProxy function, due to the onlyGuardian modifier.

modifier onlyGuardian() {
if (_msgSender() != guardian) {
revert CallerIsNotGuardian(guardian, _msgSender());
}
_;
}
/**
* @notice deploy related contract
* @dev guardian can deploy related contract
* @param _relatedContractIndex index of related contract
* @param _logic address of logic contract
* @param _data call data for logic
*/
function deployUpgradeableProxy(
uint8 _relatedContractIndex,
address _logic,
bytes memory _data
) external onlyGuardian returns (address) {
/// @dev the logic address must be a contract
if (!_logic.isContract()) {
revert LogicAddrIsNotContract(_logic);
}
​
/// @dev deploy proxy
UpgradeableProxy _proxy = new UpgradeableProxy(
_logic,
guardian,
address(this),
_data
);
relatedContracts[_relatedContractIndex] = address(_proxy);
emit RelatedContractDeployed(_relatedContractIndex, address(_proxy));
return address(_proxy);
}

Impact

Constructing a TaddleFactory with address(0x0) as a guardian results in loss of ownership of the TaddleFactory.

Proof of Concept

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

  2. Run forge test --match-contract TadleFactory

  3. Observe the result

    Ran 1 test for test/TadleFactory.t.sol:TadleFactoryTest
    [PASS] test_givenZeroAddressForGuardian_expectedRevertWithCallerIsNotGuardianWhenCallingDeployUpgradeableProxy() (gas: 791075)
    Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 348.38µs (116.83µs CPU time)
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;
​
import {Test} from "forge-std/Test.sol";
import {TadleFactory} from "../src/factory/TadleFactory.sol";
import {ITadleFactory} from "../src/factory/ITadleFactory.sol";
​
contract TadleFactoryTest is Test {
TadleFactory tadleFactory;
address zeroAddress = address(0);
​
function setUp() public {
}
​
function test_givenZeroAddressForGuardian_expectedRevertWithCallerIsNotGuardianWhenCallingDeployUpgradeableProxy() public {
tadleFactory = new TadleFactory(zeroAddress);
vm.expectRevert(abi.encodeWithSelector(ITadleFactory.CallerIsNotGuardian.selector, zeroAddress, address(this)));
​
tadleFactory.deployUpgradeableProxy(1, vm.addr(1), "");
}
}

Tools Used

Recommendations

  1. Add a check for address(0x0) in the TadleFactory::constructor

Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

[invalid] finding-Admin-Errors-Malicious

The following issues and its duplicates are invalid as admin errors/input validation/malicious intents are1 generally considered invalid based on [codehawks guidelines](https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity#findings-that-may-be-invalid). If they deploy/set inputs of the contracts appropriately, there will be no issue. Additionally admins are trusted as noted in READ.ME they can break certain assumption of the code based on their actions, and

Support

FAQs

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