Missing slippage protection in _swapDollarsForGovernance()

[0xRizwan]

Title

Missing slippage protection in _swapDollarsForGovernance()

Severity

High

Vulnerability details

LibDollarMintExcess._swapDollarsForGovernance() is used to swap dollars for Governance tokens via uniswap router.

    function _swapDollarsForGovernance(
        bytes16 amountIn
    ) internal returns (uint256) {
        address[] memory path = new address[](2);
        AppStorage storage store = LibAppStorage.appStorage();
        path[0] = store.dollarTokenAddress;
        path[1] = store.governanceTokenAddress;
        uint256[] memory amounts = _router.swapExactTokensForTokens(
            amountIn.toUInt(),
            0,                                          @audit // amountOutMin is hardcoded to 0
            path,
            address(this),
            block.timestamp + 100
        );

        return amounts[1];
    }

In above code, amountOutMin is hardcoded to 0 means means _swapDollarsForGovernance() has missing slippage protection.
The above code for swap explicitely tells that the user will accept a minimum amount of 0 output tokens from the swap, opening up the user to a catastrophic loss of funds via MEV bot sandwich attacks.

swapExactTokensForTokens() from uniswap is given below where it make sure, amount out is greater and equal to amountOutMin

    function swapExactTokensForTokens(
        uint amountIn,
        uint amountOutMin,
        address[] calldata path,
        address to,
        uint deadline
    ) external virtual override ensure(deadline) returns (uint[] memory amounts) {
        amounts = UniswapV2Library.getAmountsOut(factory, amountIn, path);
@>      require(amounts[amounts.length - 1] >= amountOutMin, 'UniswapV2Router: INSUFFICIENT_OUTPUT_AMOUNT');
        TransferHelper.safeTransferFrom(
            path[0], msg.sender, UniswapV2Library.pairFor(factory, path[0], path[1]), amounts[0]
        );
        _swap(amounts, path, to);
    }

Therefore, setting amountOutMin to 0 does not make sense as it may lead to loss of funds due to MEV sandwich attacks.

Impact

A user will get back fewer tokens than they expect, if there was a large price impact. Trades can happen at a manipulated price and end up receiving fewer tokens than current market price dictates.

Recommendations

Consider using amountOutMin as paramter instead of hardcoding to 0

[0x4007]

@rndquu rfc

[rndquu]

LibDollarMintExcess is an obsolete contract and chances are that it will be removed

I would close the current issue as "not planned" but I do agree that the slippage protection is not implemented (which is a medium severity issue)

[0x4007]

I do agree that the slippage protection is not implemented (which is a medium severity issue)

I guess @0xRizwan can redo the issue spec to focus on slippage protection and then we can fund it using our existing system so that they can receive credit?

[rndquu]

I do agree that the slippage protection is not implemented (which is a medium severity issue)

I guess @0xRizwan can redo the issue spec to focus on slippage protection and then we can fund it using our existing system so that they can receive credit?

It doesn't make sense to work on the contract that we might delete in the next big refactor

[gitcoindev]

I do agree that the slippage protection is not implemented (which is a medium severity issue)

I guess @0xRizwan can redo the issue spec to focus on slippage protection and then we can fund it using our existing system so that they can receive credit?

It doesn't make sense to work on the contract that we might delete in the next big refactor

I have the same opinion. On one hand the severity was detected, and applies here but the audit scope was mentioned multiple times in the Discord channel. I consider the finding useful, though.

[0xRizwan]

LibDollarMintExcess is an obsolete contract and chances are that it will be removed

I would close the current issue as "not planned" but I do agree that the slippage protection is not implemented (which is a medium severity issue)

The contract is a part of developement branch and was not in deprecated folder. Since sherlock audit is completed and i had high level overview of other out of scope contracts during audit so my intention was to report it before the team deploy on mainnet.

Well, i was not aware the contract is obscelate.

On severity, This is an high severity issues. If it was found at sherlock it would be 100% a high severity according to their strict rules, even other platforms like code4rena, spearbit consider slippage issues as high severity issue.

@pavlovcik Thanks for the credit thought and appreciate your response.

@gitcoindev Thanks for acknowledging the issue.

I am sure the team would fix this slippage issues in revised contract.

[rndquu]

A few notes on the current project stage.

The contracts that were part of the audit scope were carefully created/refactored by the core team with as much attention as possible hence we got a pretty solid result with 4 medium issues in the final audit report (although 0 issues result is way better).

All other contracts (related to staking, Credit token, CreditNft token) were written by 3rd party freelancers, sometimes without basic defi security skills, without any slither checks, without any proper PR reviews hence there are tons of issues in other contracts that were not part of the audit scope.

The LibDollarMintExcess library is responsible for distributing excess Dollar tokens to certain destinations (treasury, staking, etc...). It was written a long time ago, we won't use this mechanic again (and even if we are then we will rewrite the LibDollarMintExcess library from scratch).

That is why I think we'd better concentrate on the contracts that were part of the audit scope because everything else should be refactored. The next big feature we want to implement is to revise staking which includes a research of how all our obsolete staking contracts work and (ideally) migrating to a new staking contract.

@0xRizwan Thank you being proactive. There are other issues (one, two) that you might be interested to solve but touching the old contracts is like beating a dead horse.

[0xRizwan]

A few notes on the current project stage.

The contracts that were part of the audit scope were carefully created/refactored by the core team with as much attention as possible hence we got a pretty solid result with 4 medium issues in the final audit report (although 0 issues result is way better).

All other contracts (related to staking, Credit token, CreditNft token) were written by 3rd party freelancers, sometimes without basic defi security skills, without any slither checks, without any proper PR reviews hence there are tons of issues in other contracts that were not part of the audit scope.

The LibDollarMintExcess library is responsible for distributing excess Dollar tokens to certain destinations (treasury, staking, etc...). It was written a long time ago, we won't use this mechanic again (and even if we are then we will rewrite the LibDollarMintExcess library from scratch).

That is why I think we'd better concentrate on the contracts that were part of the audit scope because everything else should be refactored. The next big feature we want to implement is to revise staking which includes a research of how all our obsolete staking contracts work and (ideally) migrating to a new staking contract.

@0xRizwan Thank you being proactive. There are other issues (one, two) that you might be interested to solve but touching the old contracts is like beating a dead horse.

Thank you. I have scheduled audits from here onwards.

All the best for upcoming mainnet launch.

[rndquu]

A few notes on the current project stage.
The contracts that were part of the audit scope were carefully created/refactored by the core team with as much attention as possible hence we got a pretty solid result with 4 medium issues in the final audit report (although 0 issues result is way better).
All other contracts (related to staking, Credit token, CreditNft token) were written by 3rd party freelancers, sometimes without basic defi security skills, without any slither checks, without any proper PR reviews hence there are tons of issues in other contracts that were not part of the audit scope.
The LibDollarMintExcess library is responsible for distributing excess Dollar tokens to certain destinations (treasury, staking, etc...). It was written a long time ago, we won't use this mechanic again (and even if we are then we will rewrite the LibDollarMintExcess library from scratch).
That is why I think we'd better concentrate on the contracts that were part of the audit scope because everything else should be refactored. The next big feature we want to implement is to revise staking which includes a research of how all our obsolete staking contracts work and (ideally) migrating to a new staking contract.
@0xRizwan Thank you being proactive. There are other issues (one, two) that you might be interested to solve but touching the old contracts is like beating a dead horse.

Thank you. I have scheduled audits from here onwards.

All the best for upcoming mainnet launch.

Thank you. By the way what nickname did you use for the audit https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues ? Can't find 0xRizwan there.

[ubiquibot]

# Issue was not closed as completed. Skipping.