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

Numeric indexing in `TadleFactory` prone to errors which could result in system malfunctioning

0xsecuri

Summary

The TadleFactory contract allows the deployment of upgradeable proxies and assigns their addresses to indices in the relatedContracts mapping. This approach carries a risk of incorrect indexing, which can lead to issues like accidental overwrites which can lead to DoS or other unexpected behaviours.

Vulnerability Details

The deployUpgradeableProxy function takes a uint8 _relatedContractIndex as an index for relatedContracts to update the address of the logic contract. This approach is prone to errors because an admin might mistakenly override the wrong contract. For example, they might intend to update the contract at index 1 but accidentally press 2, leading to unintended contract updates. Mistakes with numeric indices are more likely than with string indices consist from contract names for example. Numbers are easier to mistype, such as entering 2 instead of 1. Using descriptive string names, like "SystemConfig" reduces this risk to almost impossible.

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

Impact

  1. Protocol Disruption - incorrect index assignments may disrupt services.

  2. Users might face issues if contracts are assigned incorrectly.

Tools Used

VSCode

Recommendations

Change Indexing to Strings: use string identifiers of contract names (e.g instead of index 1 => SystemConfig, just use "SystemConfig" => SystemConfig). This reduces the risk of errors, as specifying a contract name is less error-prone than using numeric indices.

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

By using string identifiers, you make it clearer and reduce the chance of accidental errors in contract indexing.

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.