search

code-423n4 / 2021-11-badgerzaps-findings

0 stars 0 forks source link

mint() Input Variable minOut Does Not Pass Value to interface ICurveFi add_liquidity() #33

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

Meta0xNull

Vulnerability details

Impact

Dev Note in Zap.sol:

User Input minOut in function mint(): function mint(IERC20 token, uint amount, uint poolId, uint idx, uint minOut)

In function mint(), another function _addLiquidity() is called but not including minOut: _addLiquidity(pool.deposit, amount, poolId + 2, idx);

In _addLiquidity(), minOut is set as 0: pool.add_liquidity(amounts, 0);

Finally it reach interface ICurveFi: function add_liquidity(uint[2] calldata amounts, uint min_mint_amount) external;

When add liquidity without capping slippage it will cause traders lose money. There is a Final Check require(_ibbtc >= minOut, "INSUFFICIENT_IBBTC") near the end of code of function mint(). But in between _addLiquidity and Final Check there are few others functions was called. If transaction fail because of Final Check happen at the end of code, it cause Traders lose lot of money especially at ETH Mainnet traders could easily lose over $100 for each transaction just because of the Final Check. 10,000 Trasactions Fail Mean Traders lose Over $1,000,000.

Proof of Concept

https://github.com/Badger-Finance/ibbtc/blob/d8b95e8d145eb196ba20033267a9ba43a17be02c/contracts/Zap.sol#L93-L104 https://github.com/Badger-Finance/ibbtc/blob/d8b95e8d145eb196ba20033267a9ba43a17be02c/contracts/Zap.sol#L125-L130 https://github.com/Badger-Finance/ibbtc/blob/d8b95e8d145eb196ba20033267a9ba43a17be02c/contracts/Zap.sol#L348-L349

Tools Used

Manual Review

Recommended Mitigation Steps

Pass the value of minOut from mint() to _addLiquidity() and then to interface ICurveFi:add_liquidity.

GalloDaSballo commented 2 years ago

Disagree with the finding, if we live in a world where the curve transactions are going to be front-run , then you also need to agree that the tech used to front-run that transaction will simulate the ibBTC Zap TX, this means that the frontrunner will notice when the withdrawal tx is going to revert.

As such, if we have a slippage check at the end, we have a guarantee that the transaction can only be front-run , up to the point specified by the slippage check

I believe the finding to be completely invalid because of the above as well as the fact that a transition is atomic in nature, you can't front-run the withdrawal from curve, without also frontrunning the slippage check, hence a frontrunner has no incentive in making the tx revert as they actually loose the front-run opportunity

0xleastwood commented 2 years ago

Agree with sponsor, MEV bots will not attempt to sandwich attack a call to a function if the resulting attack causes them to lose out on their profit, while gaining nothing in return. Marking as invalid.