rhinestone2.md

Sorry, I couldn't find an end-to-end test in your test suite for the full flow of flashloans and didnt have time myself to write it from scratch. Hence no POC this time. So in worst case this could be a false positve

Lenders Can Drain Borrowers by Replaying Borrower's Signature

Rhinestone utilizes two sets of modules for incorporating flashloan functionality between two smart accounts: FlashloanLender and FlashloanCallback (ColdStorageHook and ColdStorageFlashloan).
The flow is complex, but the following diagram from Rhinestone explains it well:
https://github.com/rhinestonewtf/core-modules/blob/main/src/ColdStorageHook/Readme.md.

To summarize:

• A borrower signs a set of operations to be executed and calls flashloan on the lender.
• The lender, in turn, calls onFlashLoan on the borrower.
• The borrower account validates the signature, executes the operations, and then approves the loan amount back to the lender.

The onFlashLoan method and the signature verification mechanism are of primary concern for this issue.

---

Vulnerable Code:

FlashloanCallback.sol

function onFlashLoan(
    address borrower,
    address token,
    uint256 amount,
    uint256, /*fee*/
    bytes calldata data
)
    external
    onlyAllowedCallbackSender
    returns (bytes32)
{
    // decode the data
    (FlashLoanType flashLoanType, bytes memory signature, Execution[] memory executions) =
        abi.decode(data, (FlashLoanType, bytes, Execution[]));
    // get the hash
    uint256 currentNonce = nonce[msg.sender][borrower];
    bytes32 hash = getTokengatedTxHash(flashLoanType, token, amount, executions, currentNonce);
    // increment the nonce
    nonce[msg.sender][borrower] = currentNonce + 1;
    // format the hash
    hash = ECDSA.toEthSignedMessageHash(hash);
    // check the signature
    bool validSig = address(msg.sender).isValidSignatureNow(hash, signature); // @audit: borrower is account, 2, executor/hooked vs account
    // if the signature is invalid, revert
    if (!validSig) revert TokenGatedTxFailed();
    // execute the flashloan

    _execute(executions);
    _execute(
        address(token), 0, abi.encodeWithSignature("approve(address,uint256)", borrower, amount)
    );

    // return the hash
    return keccak256("ERC3156FlashBorrower.onFlashLoan");
}

---

Issue Description:

While the onFlashLoan method is protected by signature verification for the borrower's signature, so lenders can not just do whatever they want by calling onFlashloan directly, there is a critical oversight:

Replaying the Signature with a Different Borrower

• Although a lender cannot directly call onFlashLoan on a borrower using the same parameters due to the nonce check, there is no "borrower" included in the hash used for signature validation.

function onFlashLoan(
    address borrower, << this input parameter
    address token,
    uint256 amount,
    uint256, /*fee*/
    bytes calldata data
)

• This allows an attacker (lender) to pass a different borrower address while replaying the signature. The digest Rhinestone constructs does not include the borrower field, so the attacker can set it to any value.
• This enables any lender to replay the signature with a different borrower and replay the executions and replay the allowances.
• Exeuction replay has its own consequences, but even if they dont, the approval would be executed anyway, allowing attacker to drain the wallet.

Consequences:

1. Draining Wallets: The spoofed borrower address would get the approved token amount. By repeating this attack with different borrower addresses while keeping the nonce in sync to original borrower, an attacker can drain the wallet for that token.
2. Execution Failures: Even if the executions fail (e.g., due to time constraints in flashloan operations), it does not halt the transaction because the return value of _execute is ignored. This further exacerbates the issue.

---

Severity Assessment:

Critical

This is a straightforward case imo, but if you would like explanation, happy to go in detail over why i think this is a critical issue.

---

Recommendation:

The straightforward solution is to include the borrower field in the hash construction.

---

Continue if above issue is valid, ignore if its not
If this issue is valid, this recommendation resolves the reported issue, but there is another interesting problem in your flashloan modules that remains unresolved.
I will report that one separately
The imapact and vulnerable function is same, but cause, path to attack and fix are different
Can you figure it out, anon ???? :)