sherlock-audit / 2024-05-beefy-cowcentrated-liquidity-manager-judging

5 stars 5 forks source link

jasonxiale - `VeloSwapUtils.swap` doesn't have slippage protection #123

Closed sherlock-admin3 closed 2 months ago

sherlock-admin3 commented 2 months ago

jasonxiale

medium

VeloSwapUtils.swap doesn't have slippage protection

Summary

The swap functions defined in VeloSwapUtils.sol set amountOutMin as 0, which means the swap function don't have slippage protection.

Vulnerability Detail

Because the first swap function is used in StrategyPassiveManagerVelodrome._chargeFees, I will take the first swap as an example.

 22     function swap(
 23         address _router,
 24         bytes memory _path,
 25         uint256 _amountIn,
 26         bool _isV3
 27     ) internal {
 28         if (_isV3) {
 29             bytes memory input = abi.encode(address(this), _amountIn, 0, _path, true);
 30             bytes[] memory inputs = new bytes[](1);
 31             inputs[0] = input;
 32             IVeloRouter(_router).execute(abi.encodePacked(V3_SWAP_EXACT_IN), inputs, block.timestamp);
 33         } else {
 34             address[] memory route = pathToRoute(_path);
 35             bytes memory input = abi.encode(address(this), _amountIn, 0, route, true);
 36             bytes[] memory inputs = new bytes[](1);
 37             inputs[0] = input;
 38 
 39             IVeloRouter(_router).execute(abi.encodePacked(V2_SWAP_EXACT_IN), inputs, block.timestamp);
 40         }
 41     }

According to StrategyPassiveManagerVelodrome.sol#L493, _isV3 is true, so the function will fall into if condition. According to the _router defined in test case, the struct of input should be:

                        address recipient;
                        uint256 amountIn;
                        uint256 amountOutMin;
                        bytes  path;
                        bool payerIsUser;

So 0 in VeloSwapUtils.sol#L29 means amountOutMin.

Impact

Because VeloSwapUtils.swap is called by StrategyPassiveManagerVelodrome._chargeFees, and _chargeFees is called by _harvest, and _harvest can be called by anyone. without properly slippage protect, it means the caller, strategist, and beefyFeeRecipient() will receive less rewards

Code Snippet

https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/cowcentrated-contracts/contracts/utils/VeloSwapUtils.sol#L22-L77

Tool used

Manual Review

Recommendation

Duplicate of #8

sherlock-admin2 commented 2 months ago

1 comment(s) were left on this issue during the judging contest.

z3s commented:

13