Cold Topaz Cottonmouth - [H-1] Using abi.encodePacked in `PredictDotLoan::_getFulfillment` function could result in Hash Collision

[sherlock-admin2]

Cold Topaz Cottonmouth

High

[H-1] Using abi.encodePacked in PredictDotLoan::_getFulfillment function could result in Hash Collision

Summary: The vulnerability in the PredictDotLoan::_getFulfillment function arises from the use of abi.encodePacked, which can lead to hash collisions. A collision occurs when two different proposals produce the same hash, resulting in the overwriting of loan data for one user by another. This can cause loss of funds, manipulation of loan stats, and incorrect loan terms for users.

Vulnerability Description: In PredictDotLoan::_getFulfillment function, the unique key is generated by using abi.encodePacked which could lead to duplication of (key) which is a unique value and represents the Loan Borrow and Lending stats of a user.
By exploiting the use of abi.encodePacked in the _getFulfillment function, attackers can create proposals that generate identical hashes, leading to the overwriting of loan data, loss of funds, and manipulation of loan stats.

Hash collisions occur when two different sets of inputs produce the same hash output, which can lead to unintended access or overwriting of critical data.

https://github.com/sherlock-audit/2024-09-predict-fun/blob/main/predict-dot-loan/contracts/PredictDotLoan.sol#L953

Impact:

Both User A and User B end up with the same hash (HashA). Since the hash is used as the key in the fulfillments mapping, User B’s fulfillment data overwrites User A’s fulfillment data, or vice versa, depending on the order of execution.

  function _getFulfillment(Proposal calldata proposal) private view
    returns (Fulfillment storage fulfillment) {
        fulfillment = fulfillments[keccak256
@>           (abi.encodePacked(proposal.from, proposal.salt, proposal.proposalType))];
    }

A might receive incorrect loan terms, mismatched collateral, or even lose access to their original loan due to the overwritten fulfillment.

Proof of Concept:

1. UserA submits a loan proposal and interacts with the contract to borrow funds. The fulfillment data is stored under HashA.
2. User B, either maliciously or accidentally, submits a proposal with carefully chosen values (like a shorter address or a salt with lots of zeros) that result in the same HashA.
3. User B then interacts with the contract, and User A’s fulfillment data gets overwritten by User B’s data.
4. When User A tries to interact with the contract again (like: to check the status of their loan or etc purpose), the contract retrieves User B’s fulfillment data due to the hash collision.

Reference Links: https://www.nethermind.io/blog/understanding-hash-collisions-abi-encodepacked-in-solidity

Recommended Mitigation:

Using abi.encode solves the issue because it adds proper separation of the data, ensuring no two distinct proposals will produce the same hash unless the inputs are exactly identical.

function _getFulfillment(Proposal calldata proposal) private view
returns (Fulfillment storage fulfillment) {
    fulfillment = fulfillments[keccak256(
-         abi.encodePacked(proposal.from, proposal.salt, proposal.proposalType))];
+        abi.encode(proposal.from, proposal.salt, proposal.proposalType))]; 
}

[ProfessorAudits]

A similar issue found and fixed : https://github.com/sherlock-audit/2022-10-nftport-judging/issues/118