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 [1702572385750] #630

Closed
c4-judge opened this issue Dec 14, 2023 · 3 comments
Closed

Upgraded Q -> 2 from #581 [1702572385750] #630

c4-judge opened this issue Dec 14, 2023 · 3 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 14, 2023
@c4-judge
Copy link
Contributor Author

Picodes marked the issue as duplicate of #128

@c4-judge
Copy link
Contributor Author

Picodes marked the issue as satisfactory

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

Picodes marked the issue as partial-50

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

1 participant