sherlock-audit / 2023-05-USSD-judging

9 stars 7 forks source link

Kose - Because of missing slippage parameter, mintForToken() can be front-runned #97

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

Kose

high

Because of missing slippage parameter, mintForToken() can be front-runned

Summary

Missing slippage parameter in mintForToken() makes it vulnerable to front-run attacks and exposes users to unwanted slippage.

Vulnerability Detail

The current implementation of the mintForToken() function lacks a parameter for controlling slippage, which makes it vulnerable to front-run attacks. Transactions involving large volumes are particularly at risk, as the minting process can be manipulated, resulting in price impact. This manipulation allows the reserves of the pool to be controlled, enabling a frontrunner to make the transferred token to appear more valuable than its actual worth. Consequently, when users mint USSD, they may receive USSD that are worth significantly less than the value of their real worth. This lack of slippage control resembles a swap without a limit on value manipulation.

Impact

User will be vulnerable to front-run attacks and receive less USSD from their expectation.

Code Snippet

USSD.sol#L150-L167

/// Mint specific AMOUNT OF STABLE by giving token
    function mintForToken(
        address token,
        uint256 tokenAmount,
        address to
    ) public returns (uint256 stableCoinAmount) {
        require(hasCollateralMint(token), "unsupported token");

        IERC20Upgradeable(token).safeTransferFrom(
            msg.sender,
            address(this),
            tokenAmount
        );
        stableCoinAmount = calculateMint(token, tokenAmount);
        _mint(to, stableCoinAmount);

        emit Mint(msg.sender, to, token, tokenAmount, stableCoinAmount);
    }

Tool used

Manual Review

Recommendation

Consider adding a minAmountOut parameter.

sherlock-admin commented 1 year ago

Removed: comment left for traceability I think this is not a valid medium. As the mintForToken function uses oracle prices, which return a weighted average price, it should not be that easy manipulated by Frontrunning.

You've deleted an escalation for this issue.
kosedogus commented 1 year ago

Escalate for 10USDC I think this is not a valid medium. As the mintForToken function uses oracle prices, which return a weighted average price, it should not be that easy manipulated by Frontrunning.

Weighted average prices does not guarantee that trades (minting in this case) executed based on the average price will be free from slippage. TWAP's are obviously vulnerable to slippage attacks. I also would like to remind that there is another valid issue due to the lack of slippage parameter in uniRouter. (That function also uses TWAP)

Shogoki commented 1 year ago

Okay, I think you are right. I removed the escalation