air_0x - abi.encodePacked Allows Hash Collision

[sherlock-admin3]

air_0x

Medium

abi.encodePacked Allows Hash Collision

Summary

From the solidity documentation: https://docs.soliditylang.org/en/v0.8.17/abi-spec.html?highlight=collisions#non-standard-packed-mode > If you use keccak256(abi.encodePacked(a, b)) and both a and b are dynamic types, it is easy to craft collisions in the hash value by moving parts of a into b and vice-versa. More specifically, abi.encodePacked("a", "bc") == abi.encodePacked("ab", "c"). This can result in the submission of values that were not actually signed

Root Cause

https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/libraries/muon/LibMuonSettlement.sol#L21

https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/libraries/muon/LibMuonSettlement.sol#L28

In function verifySettlement()

    function verifySettlement(SettlementSig memory settleSig, address partyA) internal view {
    ----_SNIP_------
@>          encodedData = abi.encodePacked(
                encodedData,  // Append the previously encoded data
                settleSig.quotesSettlementsData[i].quoteId,
                settleSig.quotesSettlementsData[i].currentPrice,
                settleSig.quotesSettlementsData[i].partyBUpnlIndex
            );
        }
@>      bytes32 hash = keccak256(

            abi.encodePacked(
                muonLayout.muonAppId,
                settleSig.reqId,
                address(this),
                "verifySettlement",
                nonces,
                AccountStorage.layout().partyANonces[partyA],
@>              encodedData,
                settleSig.upnlPartyBs,
                settleSig.upnlPartyA,
                settleSig.timestamp,
                LibMuon.getChainId()
            )
        );
        LibMuon.verifyTSSAndGateway(hash, settleSig.sigs, settleSig.gatewaySignature);
    }
}

As the solidity docs describe, two or more dynamic types are passed to abi.encodePacked. Moreover, the dynamic value settleSig is user-specified function arguments in the function above , meaning anyone can directly specify the value of these arguments when calling the function

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

This can result in the submission of values that were not actually signed

PoC

No response

Mitigation

Instead of writing functions to accept several arguments that are hashed inside the function, consider rewriting the function to take the hashed value as a function argument directly so that the hashing process happens off-chain. This approach would solve the issue and save gas.