Ironsidesec - hashProposal uses wrong typeshash when hashing the encoded Proposal struct data

[sherlock-admin4]

Ironsidesec

Medium

hashProposal uses wrong typeshash when hashing the encoded Proposal struct data

Summary

acceptLoanOfferAndFillOrder, _refinance, matchProposals use _assertValidSignature which hashes proposal data and verifies the signature. But the hashed proposal type hash computation is wrong due to usage of uint256 questionId instead of bytes32 questionId

There are 2 impacts. So, even if one is acceptable/wrong, then the issue impact on another.

1. This will break the signature verification.
2. And breaking the strict EIP712's compatibility (mentioned in readme) where atomic types should be the same as the data format in the struct. Mentioned in Definition of typed structured data section.

Root Cause

Using uint256 questionId instead of bytes32 questionId inside the type hash of hashProposal()

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

Issue flow :

1. look at line 50 below, the questionId is in bytes 32. And when hashing a proposal data, the type hash of proposal struct format should also use bytes32 for question id. But here its using uint256. Check on line 819 below.
2. Due to this, the type hash will be different result. look at the chisel example below. The hashes are different, so the signature hash is using wrong digest to verify the signature. Should have used bytes32 itself.

This breaks the EIP712 , where atomic types like uint, bytes1 to bytes32, address should be directly used. And only strings, bytes data are dynamic types, should be keccack hashed and then derive the type hash.

https://github.com/sherlock-audit/2024-09-predict-fun/blob/41e70f9eed3f00dd29aba4038544150f5b35dccb/predict-dot-loan/contracts/interfaces/IPredictDotLoan.sol#L50

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

IPredictDotLoan.sol

45:     struct Proposal {
    ---- SNIP ----
49:         QuestionType questionType;
50:   >   bytes32 questionId;
51:         bool outcome;
52:         uint256 interestRatePerSecond;
    ---- SNIP ----
59:         uint256 protocolFeeBasisPoints;
60:     }


PredictDotLoan.sol

814:     function hashProposal(Proposal calldata proposal) public view returns (bytes32 digest) {
815:         digest = _hashTypedDataV4(
816:             keccak256(
817:                 abi.encode(
818:                     keccak256(
819:      >>>                "Proposal(address from,uint256 loanAmount,uint256 collateralAmount,uint8 questionType,uint256 questionId,bool outcome,uint256 interestRatePerSecond,uint256 duration,uint256 validUntil,uint256 salt,uint256 nonce,uint8 proposalType,uint256 protocolFeeBasisPoints)"
820:                     ),
    ---- SNIP ----
824:                     proposal.questionType,
825:                     proposal.questionId,
    ---- SNIP ----

834:                 )
835:             )
836:         );
837:     }

Impact

2 impacts

1. due to wrong type hash computation leading to wrong digest validation in the signature validator, the signatures might fail.
2. breaking the EIP712 mentioned in readme where it strictly complains. The atomic types should not be hashed or converted to other types.

PoC

No response

Mitigation

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

    function hashProposal(Proposal calldata proposal) public view returns (bytes32 digest) {
        digest = _hashTypedDataV4(
            keccak256(
                abi.encode(
                   keccak256(
                       "Proposal(address from,uint256 loanAmount,uint256 collateralAmount,uint8 questionType,
- uint256 questionId,
                       bool outcome,uint256 interestRatePerSecond,uint256 duration,uint256 validUntil,uint256 salt,uint256 nonce,uint8 proposalType,uint256 protocolFeeBasisPoints)"
                    ),
                    keccak256(
                        "Proposal(address from,uint256 loanAmount,uint256 collateralAmount,uint8 questionType,
+ bytes32 questionId,
                    bool outcome,uint256 interestRatePerSecond,uint256 duration,uint256 validUntil,uint256 salt,uint256 nonce,uint8 proposalType,uint256 protocolFeeBasisPoints)"
                    ),
                    proposal.from,
    ---- SNIP ----

                )
            )
        );
    }

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
https://github.com/PredictDotFun/predict-dot-loan/pull/37