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

5 stars 2 forks source link

Amount of underlyingToken can be inflated leading to slightly higher amounts being withdrawn to a user #402

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

User deposits and get minted x amount of shares. On withdrawal(in the first instance) , the user can sacrifice a small percentage of their shares (to be converted to underlyingToken) by setting the receiver to the BathToken address :

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

On the next instance of the withdrawal, the r value will be much higher since it is calculated using :

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

which most importantly takes its calculation from :

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

Even though the shares of the caller would be lessened from the example above, the amount withdrawn will increase since the Bath token address will be holding more of the underlying tokens.

Tools Used

Recommended Mitigation Steps

Use require(receiver≠address(this), “N/A”)

bghughes commented 2 years ago

We have a withdrawal fee paid to the pool to ensure this kind of manipulation is impossible and I'm not entirely sure this is correct.

KenzoAgada commented 2 years ago

In essence, the attack vector seems to be just the user donating tokens to the contract. As far as I see this will not lead to the user being able to withdraw tokens that are not his. Issue seems invalid to me.

HickupHH3 commented 2 years ago

With the withdrawal fee, this "attack" is invalid