Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Missing related contract index validation for in TadleFactory::deployUpgradeableProxy can cause unintended contract behavior

0x5chn0uf

Summary

The TadleFactory::deployUpgradeableProxy function allows the deployment of protocol-related contracts by taking an index to determine which contract to deploy.

However, there is no validation to ensure that the provided _relatedContractIndex is within the expected range (1 to 5) and that the correct logic contract is deployed at the specified index.

Vulnerability Details

function deployUpgradeableProxy(
uint8 _relatedContractIndex,
address _logic,
bytes memory _data
) external onlyGuardian returns (address) {
@> if (!_logic.isContract()) { // @audit - What if passed an empty contract / wrong logic?
revert LogicAddrIsNotContract(_logic);
}
​
//...
@> relatedContracts[_relatedContractIndex] = address(_proxy); // @audit - Index hasn't been checked
emit RelatedContractDeployed(_relatedContractIndex, address(_proxy));
return address(_proxy);
}

This oversight can lead to unintended behavior, such as accessing uninitialized entries in the relatedContracts mapping, which may return a zero address, or deploying incorrect logic contracts, potentially leading to vulnerabilities.

Additionally, the lack of validation can also lead to unintentionally overwriting existing contracts, resulting in loss of state and functionality.

Impact

Incorrect Logic Association: If the wrong logic contract is deployed at a given index, it could lead to unexpected behavior and vulnerabilities in the system, as the contract may not function as intended.

Zero Address Issues: Providing an invalid index results in the logic being not initialized at the correct index, leading to runtime errors when attempting to interact with the contract or unexpected behavior in the system.

Contract Overwriting: Lack of index validation increases the risk of unintentionally overwriting existing contracts, potentially disrupting system functionality and causing loss of critical data.

Proof of Code:

The following test demonstrates how an invalid _relatedContractIndex can lead to unintended deployments. The first part shows deployment with an out-of-range index, while the second part shows deployment with an incorrect logic contract.

Add the following PoC to the PreMarkets.t.sol test file:

function testInvalidRelatedContractIndex() public {
TadleFactory tadleFactory = new TadleFactory(user1);
SystemConfig systemConfigLogic = new SystemConfig();
bytes memory deploy_data = abi.encodeWithSelector(
INITIALIZE_OWNERSHIP_SELECTOR,
user1
);
​
vm.startPrank(user1);
// Attempt to deploy with an invalid index succeed!
// This should revert
tadleFactory.deployUpgradeableProxy(
6, // Invalid index
address(systemConfigLogic),
deploy_data
);
​
// Attempt to deploy with an invalid logic succeed!
// This should revert
tadleFactory.deployUpgradeableProxy(
5,
address(systemConfigLogic), // Invalid logic for index
bytes(deploy_data)
);
vm.stopPrank();
​
// Index 1 is the index of SystemConfig contract
assertEq(tadleFactory.relatedContracts(1), address(0), "Contract at index 1 should not be initialized");
assertFalse(tadleFactory.relatedContracts(5) == address(0), "Contract at index 5 should be initialized");
assertFalse(tadleFactory.relatedContracts(6) == address(0), "Contract at Index 6 should be initialized");
}

Tools Used

Manual Review

Recommendations

  • Implement validation for _relatedContractIndex in the TadleFactory::deployUpgradeableProxy function to ensure it falls within the expected range (1 to 5, inclusive).

+ require(_relatedContractIndex >= 1 && _relatedContractIndex <= 5, "Invalid contract index");
​

  • Verify that the deployed logic contract corresponds to the specified index, possibly using interfaces to ensure type safety.

  • Revert the transaction if an invalid index is provided, ensuring the contract state remains consistent and secure.

Updates

Lead Judging Commences

0xnevi Lead Judge 11 months 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.