Sushi Miso contest
Findings & Analysis Report

2021-11-05

Overview

About C4

Code 432n4 (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 code contest is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the code contest outlined in this document, C4 conducted an analysis of the Sushi Miso contest smart contract system written in Solidity. The code contest took place between September 9—September 15 2021.

Wardens

11 Wardens contributed reports to the Sushi Miso contest code contest:

This contest was judged by ghoul.sol.

Final report assembled by itsmetechjay and CloudEllie.

Summary

The C4 analysis yielded an aggregated total of 25 unique vulnerabilities and 101 total findings. All of the issues presented here are linked back to their original finding.

Of these vulnerabilities, 3 received a risk rating in the category of HIGH severity, 1 received a risk rating in the category of MEDIUM severity, and 21 received a risk rating in the category of LOW severity.

C4 analysis also identified 47 non-critical recommendations and 29 gas optimizations.

Scope

The code under review can be found within the C4 Sushi Miso contest repository and is composed of 106 smart contracts written in the Solidity programming language, and includes 4,040 lines of Solidity code.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities according to a methodology based on OWASP standards.

Vulnerabilities are divided into three primary risk categories: high, medium, and low.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

  • Malicious Input Handling
  • Escalation of privileges
  • Arithmetic
  • Gas use

Further information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website.

High Risk Findings (3)

[H-01] PostAuctionLauncher.sol#finalize() Adding liquidity to an existing pool may allows the attacker to steal most of the tokens

Submitted by WatchPug, also found by 0xRajeev and cmichel.

PostAuctionLauncher.finalize() can be called by anyone, and it sends tokens directly to the pair pool to mint liquidity, even when the pair pool exists.

An attacker may control the LP price by creating the pool and then call finalize() to mint LP token with unfair price (pay huge amounts of tokens and get few amounts of LP token), and then remove the initial liquidity they acquired when creating the pool and take out huge amounts of tokens.

https://github.com/sushiswap/miso/blob/2cdb1486a55ded55c81898b7be8811cb68cfda9e/contracts/Liquidity/PostAuctionLauncher.sol#L257

/**
 * @notice Finalizes Token sale and launches LP.
 * @return liquidity Number of LPs.
 */
function finalize() external nonReentrant returns (uint256 liquidity) {
    // GP: Can we remove admin, let anyone can finalise and launch?
    // require(hasAdminRole(msg.sender) || hasOperatorRole(msg.sender), "PostAuction: Sender must be operator");
    require(marketConnected(), "PostAuction: Auction must have this launcher address set as the destination wallet");
    require(!launcherInfo.launched);

    if (!market.finalized()) {
        market.finalize();
    }
    require(market.finalized());

    launcherInfo.launched = true;
    if (!market.auctionSuccessful() ) {
        return 0;
    }

    /// @dev if the auction is settled in weth, wrap any contract balance 
    uint256 launcherBalance = address(this).balance;
    if (launcherBalance > 0 ) {
        IWETH(weth).deposit{value : launcherBalance}();
    }
    
    (uint256 token1Amount, uint256 token2Amount) =  getTokenAmounts();

    /// @dev cannot start a liquidity pool with no tokens on either side
    if (token1Amount == 0 || token2Amount == 0 ) {
        return 0;
    }

    address pair = factory.getPair(address(token1), address(token2));
    if(pair == address(0)) {
        createPool();
    }

    /// @dev add liquidity to pool via the pair directly
    _safeTransfer(address(token1), tokenPair, token1Amount);
    _safeTransfer(address(token2), tokenPair, token2Amount);
    liquidity = IUniswapV2Pair(tokenPair).mint(address(this));
    launcherInfo.liquidityAdded = BoringMath.to128(uint256(launcherInfo.liquidityAdded).add(liquidity));

    /// @dev if unlock time not yet set, add it.
    if (launcherInfo.unlock == 0 ) {
        launcherInfo.unlock = BoringMath.to64(block.timestamp + uint256(launcherInfo.locktime));
    }
    emit LiquidityAdded(liquidity);
}

In line 257, PostAuctionLauncher will mint LP with token1Amount and token2Amount. The amounts (token1Amount and token2Amount) are computed according to the auction result, without considering the current price (reserves) of the existing tokenPair.

See PostAuctionLauncher.getTokenAmounts()

PostAuctionLauncher will receive an unfairly low amount of lp token because the amounts sent to tokenPair didn’t match the current price of the pair.

See UniswapV2Pair.mint(…)

liquidity = MathUniswap.min(amount0.mul(_totalSupply) / _reserve0, amount1.mul(_totalSupply) / _reserve1);

Impact

Lose a majority share of the tokens.

Proof of Concept

  1. The attacker creates LP with 0.0000001 token1 and 1000 token2, receives 0.01 LP token;
  2. Call PostAuctionLauncher.finalize(). PostAuctionLauncher will mint liquidity with 2000 token1 and 1000 token2 for example, receives only 0.01 LP token;
  3. The attacker removes all his LP, receives 1000 token1 (most of which come from PostAuctionLauncher).

Recommended Mitigation Steps

To only support tokenPair created by PostAuctionLauncher or check for the token price before mint liquidity.

Clearwood (Sushi Miso) confirmed and patched:

https://github.com/sushiswap/miso/pull/21

[H-02] SushiToken transfers are broken due to wrong delegates accounting on transfers

Submitted by cmichel.

When minting / transferring / burning tokens, the SushiToken._beforeTokenTransfer function is called and supposed to correctly shift the voting power due to the increase/decrease in tokens for the from and to accounts. However, it does not correctly do that, it tries to shift the votes from the from account, instead of the _delegates[from] account. This can lead to transfers reverting.

Proof Of Concept

Imagine the following transactions on the SushiToken contract. We’ll illustrate the corresponding _moveDelegates calls and written checkpoints for each.

  • mint(A, 1000) = transfer(0, A, 1000) => _moveDelegates(0, delegates[A]=0) => no checkpoints are written to anyone because delegatees are still zero
  • A delegates to A’ => _moveDelegates(0, A') => writeCheckpoint(A', 1000)
  • B delegates to B’ => no checkpoints are written as B has a zero balance
  • transfer(A, B, 1000) => _moveDelegates(A, delegates[B] = B') => underflows when subtracting amount=1000 from A’s non-existent checkpoint (defaults to 0 votes)

It should subtract from A’s delegatee A'’s checkpoint instead.

Impact

Users that delegated votes will be unable to transfer any of their tokens.

Recommended Mitigation Steps

In SushiToken._beforeTokenTransfer, change the _moveDelegates call to be from _delegates[from] instead:

function _beforeTokenTransfer(address from, address to, uint256 amount) internal override { 
    _moveDelegates(_delegates[from], _delegates[to], amount);
    super._beforeTokenTransfer(from, to, amount);
}

This is also how the original code from Compound does it.

maxsam4 (Sushi Miso) acknowledged:

This is a known issue in Sushi token but was kept unchanged in MISO for “preservation of history :)“. That was not necessarily a wise choice lol. I think 1 severity should be fine for this as this was an intentional thing. The delegate feature is not supposed to be used in these tokens. We might create a new token type with this bug fixed.

ghoul-sol (judge) commented:

We have crazy wallets on the blockchain that will call every possible function available to them and that’s why I’m keeping this as is. Even intentional, the issue stands so the warden should get credit for it.

[H-03] Last person to withdraw his tokens might not be able to do this, in Crowdsale (edge case)

Submitted by gpersoon.

Impact

Suppose a Crowdsale is successful and enough commitments are made before the marketInfo.endTime. Suppose marketStatus.commitmentsTotal == marketInfo.totalTokens -1 // note this is an edge case, but can be constructed by an attacker Then the function auctionEnded() returns true Assume auctionSuccessful() is also true (might depend on the config of marketPrice.goal and marketInfo.totalTokens) Then an admin can call finalize() to finalize the Crowdsale. The function finalize distributes the funds and the unsold tokens and sets status.finalized = true so that finalized cannot be called again. Now we have “marketInfo.totalTokens -1” tokens left in the contract

However commitEth() or commitTokens() can still be called (they give no error message that the auction has ended) Then functions call calculateCommitment, which luckily prevent from buying too much, however 1 token can still be bought These functions also call \_addCommitment(), which only checks for marketInfo.endTime, which hasn’t passed yet.

Now an extra token is sold and the contract has 1 token short. So the last person to withdraw his tokens cannot withdraw them (because you cannot specify how much you want to withdraw)

Also the revenues for the last token cannot be retrieved as finalize() cannot be called again.

Proof of Concept

https://github.com/sushiswap/miso/blob/master/contracts/Auctions/Crowdsale.sol#L374

 function finalize() public nonReentrant {
        require(hasAdminRole(msg.sender) || wallet == msg.sender || hasSmartContractRole(msg.sender) || finalizeTimeExpired(),"Crowdsale: sender must be an admin"); // can be called by admin
        MarketStatus storage status = marketStatus;
        require(!status.finalized, "Crowdsale: already finalized");
        MarketInfo storage info = marketInfo;
        require(auctionEnded(), "Crowdsale: Has not finished yet");    // is true if enough sold, even if this is before marketInfo.endTime

        if (auctionSuccessful()) {          
            /// @dev Transfer contributed tokens to wallet.
            /// @dev Transfer unsold tokens to wallet.
        } else {
            /// @dev Return auction tokens back to wallet.
        }
        status.finalized = true;

function auctionEnded() public view returns (bool) {
        return block.timestamp > uint256(marketInfo.endTime) || 
        _getTokenAmount(uint256(marketStatus.commitmentsTotal) + 1) >= uint256(marketInfo.totalTokens); // is true if enough sold, even if this is before marketInfo.endTime
    }

function auctionSuccessful() public view returns (bool) {
        return uint256(marketStatus.commitmentsTotal) >= uint256(marketPrice.goal);
}

function commitEth(address payable _beneficiary, bool readAndAgreedToMarketParticipationAgreement ) public payable nonReentrant  {
       ...
        uint256 ethToTransfer = calculateCommitment(msg.value);
       ...
       _addCommitment(_beneficiary, ethToTransfer);
   
 function calculateCommitment(uint256 _commitment) public view returns (uint256 committed) { // this prevents buying too much
        uint256 tokens = _getTokenAmount(_commitment);
        uint256 tokensCommited =_getTokenAmount(uint256(marketStatus.commitmentsTotal));
        if ( tokensCommited.add(tokens) > uint256(marketInfo.totalTokens)) {
            return _getTokenPrice(uint256(marketInfo.totalTokens).sub(tokensCommited));
        }
        return _commitment;
    }

function _addCommitment(address _addr, uint256 _commitment) internal {
        require(block.timestamp >= uint256(marketInfo.startTime) && block.timestamp <= uint256(marketInfo.endTime), "Crowdsale: outside auction hours"); // doesn't check auctionEnded() nor status.finalized
        ...
        uint256 newCommitment = commitments[_addr].add(_commitment);
        ...
        commitments[_addr] = newCommitment;

function withdrawTokens(address payable beneficiary) public   nonReentrant  {    
        if (auctionSuccessful()) {
            ...
            uint256 tokensToClaim = tokensClaimable(beneficiary);
            ...
            claimed[beneficiary] = claimed[beneficiary].add(tokensToClaim);
            _safeTokenPayment(auctionToken, beneficiary, tokensToClaim);    // will fail is last token is missing
        } else {



## Tools Used

## Recommended Mitigation Steps
In the function _addCommitment, add a check on auctionEnded() or status.finalized

Clearwood (Sushi Miso) confirmed and patched:

https://github.com/sushiswap/miso/pull/20

Medium Risk Findings (1)

[M-01] use of transfer() instead of call() to send eth

Submitted by JMukesh.

Impact

Use of transfer() might render ETH impossible to withdraw because after istanbul hardfork, there is an increase in the gas cost of the SLOAD operation and therefore breaks some existing smart contracts.Those contracts will break because their fallback functions used to consume less than 2300 gas, and they’ll now consume more, since 2300 the amount of gas a contract’s fallback function receives if it’s called via Solidity’s transfer() or send() methods. Any smart contract that uses transfer() or send() is taking a hard dependency on gas costs by forwarding a fixed amount of gas: 2300.

Proof of Concept

Tools Used

manual review

Recommended Mitigation Steps

use call() to send eth

maxsam4 (Sushi Miso) disputed and commented:

This is intentional, not a risk. The contract does not want to give any gas stipend to the destination.

Even if the user messes up, misoDev address can be changed to a proper address later.

ghoul-sol (judge) commented:

using .transfer can make ETH transfer to a smart contract impossible. User can always change the address however I agree with warden that this is an issue.

Low Risk Findings (21)

Non-Critical Findings (47)

Gas Optimizations (29)

Disclosures

C4 is an open organization governed by participants in the community.

C4 Contests incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Contest submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.

C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.