Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handling External Calls to IERC20Metadata.decimals() Failing Silently #136

Open
c4-bot-4 opened this issue Oct 27, 2024 · 0 comments
Open
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working 🤖_primary AI based primary recommendation sufficient quality report This report is of sufficient quality

Comments

@c4-bot-4
Copy link
Contributor

Lines of code

https://github.com/ronin-chain/katana-v3-contracts/blob/03c80179e04f40d96f06c451ea494bb18f2a58fc/src/periphery/NonfungibleTokenPositionDescriptor.sol#L75-L76

Vulnerability details

Proof of Concept

Explain and rationalize the potential impact. Provide:

  • Direct links to all referenced code in GitHub
  • Screenshots, logs, or any other relevant proof that illustrates the concept
  • Any additional research or code needed to validate the claims made in your submission

Issue: Handling External Calls to IERC20Metadata.decimals() Failing Silently

Severity Level: Medium

Issue Type: Data Integrity and External Call Validation

Impact

The contract relies on external calls to token contracts, such as calling IERC20Metadata(token).decimals(), to retrieve token metadata. There is a risk that these external calls could fail, revert, or return unexpected data due to issues such as:

•	Poorly implemented token contracts
•	Malicious contracts intentionally returning incorrect data
•	Changes in the interface or behavior of a token contract

If these calls fail or return incorrect data, the contract could generate incorrect metadata for NFTs, which may mislead users or disrupt the intended functionality. For example, if decimals() fails or returns an unreasonable value, the constructed token URI could include inaccurate token details, leading to confusion or misrepresentation.

Explanation

•	External Call Risks: The contract interacts with external contracts by calling IERC20Metadata functions. If these contracts are not properly implemented or have been manipulated, they could cause the external calls to fail. This can occur due to network conditions, gas limits, or other issues beyond the control of the contract.
•	Potential Impact: When the decimals() function fails, it may cause incorrect or default values to be used in calculations, resulting in incorrect metadata in the token URI. This could misrepresent the NFT’s properties.
•	Silent Failures: Without explicit error handling, failures in external calls may not be detected, allowing the contract to continue execution with default or zero values. This may cause logical inconsistencies and inaccurate data representation.
•	Impact: The issue does not directly compromise user funds or grant unauthorized access but can result in significant data integrity issues.
•	Likelihood: While ERC20 token standards are widely followed, the risk remains that an external contract may not adhere to the expected interface, or there may be an error that could affect the metadata generation.

Proof of Concept (PoC)

The following PoC demonstrates how an external call to IERC20Metadata.decimals() could fail or return unexpected results:

PoC

In this example, if IERC20Metadata(token).decimals() fails, the catch block handles the error by returning a default value of 18.

Mitigation

Implement error handling using try/catch when making external calls to ensure the contract handles cases where the call fails gracefully. This can prevent logical inconsistencies and improve the robustness of the contract.

Solidity Coded Mitigation

Mitigation

In this mitigation, the try/catch block ensures that if the call fails, the contract returns a default value of 18 decimals. Additionally, the require statement validates that the returned decimals value is within a reasonable range, mitigating risks of a malicious contract returning an unusual value.

References

1.	Solidity Documentation on Try/Catch
2.	OpenZeppelin Contracts - Error Handling
3.	ERC20 Standard Documentation

Summary

Adding checks to ensure external calls do not fail silently improves the robustness of the contract by handling cases where the call may revert or return unexpected values. By using try/catch, the contract can provide a fallback mechanism and validate data integrity, enhancing the contract’s reliability and preventing potential data inconsistencies.

Recommended Mitigation Steps

Assessed type

Invalid Validation

@c4-bot-4 c4-bot-4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Oct 27, 2024
c4-bot-3 added a commit that referenced this issue Oct 27, 2024
@c4-bot-12 c4-bot-12 added the 🤖_primary AI based primary recommendation label Oct 30, 2024
howlbot-integration bot added a commit that referenced this issue Nov 4, 2024
@howlbot-integration howlbot-integration bot added the sufficient quality report This report is of sufficient quality label Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working 🤖_primary AI based primary recommendation sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

2 participants