code-423n4 / 2022-05-rubicon-findings

5 stars 2 forks source link

Cannot deposit to BathToken if token is Deflationary Token (BathHouse.sol) #126

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathHouse.sol#L136-L203

Vulnerability details

Impact

Function openBathTokenSpawnAndSignal will alway revert when newBathTokenUnderlying or desiredPairedAsset is deflationary token

Proof of Concept

There are ERC20 tokens that may make certain customizations to their ERC20 contracts. One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom() For example, I will assume that newBathTokenUnderlying is deflationary token. After line 163, the actual amount of newBathTokenUnderlying that BathHouse gained will be smaller than initialLiquidityNew. It will make the deposit call reverted because there are not enough fund to transfer.

Tools Used

Manual review

Recommended Mitigation Steps

set initialLiquidityNew = newBathTokenUnderlying.balanceOf(address(this)) after line 163 and initialLiquidityExistingBathToken = desiredPairedAsset.balanceOf(address(this)) after line 178

bghughes commented 2 years ago

This is correct, though I believe un needed. If the user wants to create a vault for a deflationary token they need only account for said transfer fee when calculating their initialLiquidityNew value.

HickupHH3 commented 2 years ago

Not sure how you can account for transfer fee in initialLiquidityNew since it's the same amount used for approval and deposit: IBathToken(newOne).deposit(initialLiquidityNew, msg.sender);

It simply means that deflationary / FoT tokens arent supported at all, which isn't necessarily a bad thing. There isn't a loss of assets, though function of the protocol or its availability could be impacted. Keeping it at medium severity, although could've potentially lowered to QA too.