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

[LOW] Lack of Check in `TadleFactory::deployUpgradeableProxy()` to Prevent Overwriting Addresses in `relatedContracts` Mapping.

justawanderkid

Description

The deployUpgradeableProxy function in the TadleFactory contract allows the guardian to deploy a new upgradeable proxy and store its address in the relatedContracts mapping. However, it lacks a check to ensure the index is not already set, which can lead to accidental overwriting of existing contract addresses with not Related Contracts for the given index.

Impact

if TadleFactory::guardian accidently overwrite a index in relatedContracts mapping without paying attention, then whenever we use RelatedContractLibraries contract getter functions inside our core contracts, the protocol will not work as expected and this will cause issues.

Proof of Concept

as it's stated in TadleFactory Contract, the 1 index from relatedContracts mapping, should return SystemConfig and 4 index should return CapitalPool, Can TadleFactory::guardian
accidently mess this up? yes.

lets overwrite the 1 index of relatedContracts mapping with CapitalPool proxy address.

function test_guardian_overwrites_relatedContracts_mapping() external {
vm.startPrank(user1); // user1 is `owner()` of tadleFactory.
​
TadleFactory tadleFactory = new TadleFactory(user1);
​
SystemConfig systemConfigLogic = new SystemConfig(); // SystemConfig must be set on index 1 `relatedContracts` mapping
CapitalPool capitalPoolLogic = new CapitalPool(); // CapitalPool must be set on index 2 of `relatedContracts` mapping
​
bytes memory deploy_data = abi.encodeWithSelector( INITIALIZE_OWNERSHIP_SELECTOR, user1);
​
tadleFactory.deployUpgradeableProxy( 1, address(systemConfigLogic), bytes(deploy_data));
address capitalPoolProxy = tadleFactory.deployUpgradeableProxy( 1, address(capitalPoolLogic), bytes(deploy_data)); // we overwrite the index `1` of `relatedContracts` mapping with `capitalPoolLogic`.
​
assertEq(tadleFactory.relatedContracts(1), address(capitalPoolProxy)); // get index `1` from `relatedContracts` mapping and compare the address to `capitalPoolLogic`.
​
vm.stopPrank();
}

run the test with following command:

forge test --match-test test_guardian_overwrites_relatedContracts_mapping -vvvv

we successfully overwrite 1 index of relatedContracts mapping and set it to capitalPoolProxy address.

Recommended Mitigation

Add a check to ensure the given index is not already in use:

function deployUpgradeableProxy(uint8 _relatedContractIndex, address _logic, bytes memory _data) external onlyGuardian returns (address) {
if (!_logic.isContract()) {
revert LogicAddrIsNotContract(_logic);
}
​
+ if (relatedContracts[_relatedContractIndex] != address(0)) {
+ revert IndexAlreadyInUse(_relatedContractIndex);
+ }
​
UpgradeableProxy _proxy = new UpgradeableProxy(
_logic,
guardian,
address(this),
_data
);
​
relatedContracts[_relatedContractIndex] = address(_proxy);
emit RelatedContractDeployed(_relatedContractIndex, address(_proxy));
return address(_proxy);
}

This ensures existing addresses are not overwritten, maintaining data integrity and security.

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.