[SLG-05M] Insecure Loan Key Generation

SLG-05M: Insecure Loan Key Generation

Type	Severity	Location
Logical Fault		SalvorLending.sol:L462, L464

Description:

The SalvorLending contract will generate unique keys per loan to be able to track their fulfilment state, however, the way these keys are generated are prone to collisions and generally insecure.

Impact:

It is presently possible to hijack the fulfilment of a loan offer trivially by using the same NFT contract and salt even though the token ID of the hijacked loan offer may not be held by the user.

Example:

/**
* @notice Validates a loan offer and corresponding token. This internal function ensures the loan offer and token meet various criteria including active pool, valid token signature, matching salts, sender authenticity, and signature expiry.
* @param _loanOffer The loan offer to validate.
* @param _token The token associated with the loan offer.
* @param _tokenSignature The signature of the token.
*/
function validateLoanOffer(LibLending.LoanOffer memory _loanOffer, LibLending.Token memory _token, bytes memory _tokenSignature) internal {
    require(_loanOffer.amount > 0, "lend amount cannot be 0");
    require(lendingPools[_loanOffer.nftContractAddress].isActive, "pool is not active");
    require(_hashTypedDataV4(LibLending.hashToken(_token)).recover(_tokenSignature) == validator, "token signature is not valid");
    require(keccak256(abi.encodePacked(_token.salt)) == keccak256(abi.encodePacked(_loanOffer.salt)), "salt does not match");
    require(_token.sender == msg.sender, "token signature does not belong to msg.sender");
    require(_token.blockNumber + blockRange > block.number, "token signature has been expired");

    bytes32 hash = LibLending.hashKey(_loanOffer);

    require(_loanOffer.size > sizes[hash], "size is filled");
    sizes[hash] += 1;
}

Recommendation:

We advise the code to utilize the full loan offer hashes as keys instead of the constructed ones via LibLending::hashKey, ensuring that a single adjustment in the loan offer data will lead to an entirely different fulfilment entry being queried.

salvorio / salvor-contracts