062.md

Dandy Caramel Tortoise

Medium

Incorrect messge sender in case base _trustedForwarder is used

Summary

The last 20 bytes could be used to identify both the market forwarder and also the market user in case the market forwarder uses the base forwarder themselves

Root Cause

_msgSenderForMarket decodes the last 20 bytes as message sender in case the current sender is a trusted forwarder for the market

https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2Context.sol#L122-L140

    function _msgSenderForMarket(uint256 _marketId)
        internal
        view
        virtual
        returns (address)
    {
        if (
            msg.data.length >= 20 &&
            isTrustedMarketForwarder(_marketId, _msgSender())
        ) {
            address sender;
            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();
    }

But in case the trusted forwarder themselves is using the base trusted forwarder ie. _trustedForwarder, the last 20 bytes will be the address of the market's trusted forwarder https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/ERC2771ContextUpgradeable.sol#L34-L50

    function _msgSender()
        internal
        view
        virtual
        override
        returns (address sender)
    {
        if (isTrustedForwarder(msg.sender)) {
            // The assembly code is more direct than the Solidity version using `abi.decode`.
            assembly {
                sender := shr(96, calldataload(sub(calldatasize(), 20)))
            }
        } else {
            return super._msgSender();
        }
    }

Hence in such a scenario the decoding will be incorrect

Internal pre-conditions

A market's trusted forwarder should use the base trusted forwarder themselves

External pre-conditions

No response

Attack Path

No response

Impact

Incorrect decoding of the message sender causing fund transfer etc. to happen from the incorrect account

PoC

No response

Mitigation

Avoid using base trusted forwarder by market trusted providers