code423n4 commented 3 years ago

Handle

gpersoon

Vulnerability details

Impact

Suppose some ERC20 tokens accidentally end up in the MISOMarket contract.

Make sure you have 1 of those tokens before calling createMarket. Then you can call createMarket with the same ERC20 token and _tokenSupply == 1 and otherwise valid /constructed parameters (for example based on a previous call to createMarket ).

createMarket will transfer 1 ERC20 token to itself. Assume the "data" parameter refers to a different token so initMarket() doesn't touch the tokens. The createMarket will send the entire balance of the ERC20 tokens back to the caller of createMarket.

Proof of Concept

https://github.com/sushiswap/miso/blob/master/contracts/MISOMarket.sol#L273

  function createMarket(uint256 _templateId, address _token, uint256 _tokenSupply,address payable _integratorFeeAccount,bytes calldata _data) external payable returns (address newMarket) {
        ...
        if (_tokenSupply > 0) {
            _safeTransferFrom(_token, msg.sender, _tokenSupply);
            require(IERC20(_token).approve(newMarket, _tokenSupply), "1");
        }
        IMisoMarket(newMarket).initMarket(_data);
        if (_tokenSupply > 0) {
            uint256 remainingBalance = IERC20(_token).balanceOf(address(this));
            if (remainingBalance > 0) {
                _safeTransfer(_token, msg.sender, remainingBalance);
            }
        }

Tools Used

Recommended Mitigation Steps

Check the initial balance of the ERC20 token and never send back more than supplied.

maxsam4 commented 3 years ago

No assets should be ever held by this contract. This method can be considered similar to sweep functionality in other contracts to recover mistakenly sent tokens but if anyone sends tokens directly to this contract, they can be assumed lost.

ghoul-sol commented 3 years ago

There's no exploit while properly using the contract. Per sponsor comment, invalid

code-423n4 / 2021-09-sushimiso-findings