merlin - The SmartCommitmentForwarder smart contract isn't compatible with FlashRolloverLoan_G5.sol

[sherlock-admin2]

merlin

medium

The SmartCommitmentForwarder smart contract isn't compatible with FlashRolloverLoan_G5.sol

Summary

As per the code, invoking SmartCommitmentForwarder.acceptCommitmentWithRecipient() from FlashRolloverLoan_G5 will result in the DOS.

Vulnerability Detail

The issue lies in the fact that SmartCommitmentForwarder operates with msg.sender, which in our case is FlashRolloverLoan_G5, and not the borrower directly:

function _msgSender() internal view virtual returns (address) {
        return msg.sender;
    }

So, when FlashRolloverLoan_G5 calls the acceptCommitmentWithRecipient function, it triggers the internal _submitBidWithCollateral function in SmartCommitmentForwarder.sol and pass _msgSender(FlashRolloverLoan_G5) as the borrower:

bidId = _submitBidWithCollateral(createLoanArgs, _msgSender());

This causes a revert in TellerV2Context because _msgSender == FlashRolloverLoan_G5, leading to the error message Sender must approve market forwarder:

function _msgSenderForMarket(
        uint256 _marketId
    ) internal view virtual returns (address) {
        if (
            msg.data.length >= 20 &&
            isTrustedMarketForwarder(_marketId, _msgSender())
        ) {
            address sender; // borrower and _msgSender() = Smart_Forwarder
            assembly {
                sender := shr(96, calldataload(sub(calldatasize(), 20)))
            }
            // Ensure the appended sender address approved the forwarder
            require(
-->             _approvedForwarderSenders[_msgSender()].contains(sender),
                "Sender must approve market forwarder" 
            );
            return sender;
        }

        return _msgSender();
    }

Impact

The borrower can't interact with the acceptCommitmentWithRecipient function in SmartCommitmentForwarder.sol through FlashRolloverLoan_G5.sol.

Code Snippet

contracts/contracts/LenderCommitmentForwarder/extensions/FlashRolloverLoan_G5.sol#L287-L305 contracts/contracts/LenderCommitmentForwarder/SmartCommitmentForwarder.sol#L106

Tool used

Manual Review

Recommendation

Consider overriding the _msgSender() function in SmartCommitmentForwarder.sol and implementing it in the same way as in LenderCommitmentForwarder_G3 using ExtensionsContextUpgradeable.sol:

+function _msgSender()
+        internal
+        view
+        virtual
+        override(ContextUpgradeable, ExtensionsContextUpgradeable)
+        returns (address sender)
+    {
+        return ExtensionsContextUpgradeable._msgSender();
+    }    

Duplicate of #138