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

Upgraded Q -> 2 from #581 [1703633976168] #640

Closed
c4-judge opened this issue Dec 26, 2023 · 4 comments
Closed

Upgraded Q -> 2 from #581 [1703633976168] #640

c4-judge opened this issue Dec 26, 2023 · 4 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value duplicate-247 partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%)

Comments

@c4-judge
Copy link
Contributor

Judge has assessed an item in Issue #581 as 2 risk. The relevant finding follows:

[L-08] Address Collision Risk in Callback Validation Logic**
Contract: CallbackLib.sol
The validateCallback function employs deterministic address computation to verify the authenticity of a Uniswap pool. This approach is based on the assumption that the address derived from the contract’s creation bytecode and the creator’s address is unique. However, this method is susceptible to address collision risks, where a different contract, under specific circumstances, might share the same computed address, leading to erroneous validation.

Suggested Fix:
Strengthen the validation logic by incorporating additional checks beyond address computation. This could involve maintaining a registry of verified pool addresses or introducing cryptographic verification methods that ensure the uniqueness and legitimacy of the pool beyond its address.

Code Snippet:
function validateCallback(
address sender,
address factory,
PoolFeatures memory features
) internal pure {
// existing logic to compute and compare addresses
if (
address(
uint160(
uint256(
keccak256(
abi.encodePacked(
bytes1(0xff),
factory,
keccak256(abi.encode(features)),
Constants.V3POOL_INIT_CODE_HASH
)
)
)
)
) != sender
) revert Errors.InvalidUniswapCallback();
}

@c4-judge c4-judge added the 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value label Dec 26, 2023
@c4-judge
Copy link
Contributor Author

Picodes marked the issue as duplicate of #247

@c4-judge c4-judge added duplicate-247 satisfactory satisfies C4 submission criteria; eligible for awards labels Dec 26, 2023
@c4-judge
Copy link
Contributor Author

Picodes marked the issue as satisfactory

@c4-judge
Copy link
Contributor Author

Picodes marked the issue as partial-50

@c4-judge c4-judge added partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) and removed satisfactory satisfies C4 submission criteria; eligible for awards labels Dec 26, 2023
@Picodes
Copy link

Picodes commented Dec 26, 2023

Giving partial credit as this report doesn't discuss the feasability

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 duplicate-247 partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%)
Projects
None yet
Development

No branches or pull requests

2 participants