Users will lose value if paymentToken is a fee-on-transfer or a rebasing token
Summary
If fee-on-transfer or rebasing token is used as paymentToken in a Market, then the logic in settlePoolUserRedeems will work incorrectly and will revert for the last users to call it, since there won't be enough liquidity in the liquidity manager.
Vulnerability Detail
Some tokens take a transfer fee (e.g. STA, PAXG), some do not currently charge a fee but may do so in the future (e.g. USDT, USDC). Notice: USDC is named specifically to be used in the protocol docs.
Also, some tokens may make arbitrary balance modifications outside of transfers (e.g. Ampleforth style rebasing tokens, Compound style airdrops of governance tokens, mintable / burnable tokens).
In both cases, if a protocol caches the amount transferred (as MarketCore does with userAction.amount) then the contract might be operating with wrong or outdated values for the balance of a user.
When a user calls any of the mint methods, he passes an amount argument of the amount of paymentTokens to be transferred out of his wallet. The code then caches that user provided amount with userAction.amount += amount; even though this might not be the actual amount transferred (or in case of a rebasing token, this can be an outdated value later).
Now when a user calls the settlePoolUserRedeems() function, the code does the following:
uint256 amountPaymentTokenToSend = uint256(userAction.amount).mul(poolToken_price);
and transfers out amountPaymentTokenToSend to him. If the liquidityManager did not actually contain userAction.amount of tokens, this can result in the last users calling settlePoolUserRedeems being unable to do so, because the transfer from the liquidityManager to them reverts due to insufficient liquidity.
Impact
Impact will be a loss of value for users due to DoS on settlePoolUserRedeems. Putting this as High severity, because USDC is explicitly pointed out in docs to be used, but it is an upgradeable contract which already has hooks implemented into it and it is possible that it adds a fee in the future.
For fee-on-transfer tokens, it is best to check the balance before and after the transfer, to see the actually received value. For rebasing tokens it's best to add an allowlist functionality for ERC20 tokens.
pashov
high
Users will lose value if
paymentTokenis a fee-on-transfer or a rebasing tokenSummary
If fee-on-transfer or rebasing token is used as
paymentTokenin a Market, then the logic insettlePoolUserRedeemswill work incorrectly and will revert for the last users to call it, since there won't be enough liquidity in the liquidity manager.Vulnerability Detail
Some tokens take a transfer fee (e.g. STA, PAXG), some do not currently charge a fee but may do so in the future (e.g. USDT, USDC). Notice:
USDCis named specifically to be used in the protocol docs. Also, some tokens may make arbitrary balance modifications outside of transfers (e.g. Ampleforth style rebasing tokens, Compound style airdrops of governance tokens, mintable / burnable tokens).In both cases, if a protocol caches the amount transferred (as
MarketCoredoes withuserAction.amount) then the contract might be operating with wrong or outdated values for the balance of a user. When a user calls any of themintmethods, he passes anamountargument of the amount of paymentTokens to be transferred out of his wallet. The code then caches that user provided amount withuserAction.amount += amount;even though this might not be the actual amount transferred (or in case of a rebasing token, this can be an outdated value later). Now when a user calls thesettlePoolUserRedeems()function, the code does the following:uint256 amountPaymentTokenToSend = uint256(userAction.amount).mul(poolToken_price);and transfers outamountPaymentTokenToSendto him. If theliquidityManagerdid not actually containuserAction.amountof tokens, this can result in the last users callingsettlePoolUserRedeemsbeing unable to do so, because the transfer from theliquidityManagerto them reverts due to insufficient liquidity.Impact
Impact will be a loss of value for users due to DoS on
settlePoolUserRedeems. Putting this asHighseverity, becauseUSDCis explicitly pointed out in docs to be used, but it is an upgradeable contract which already has hooks implemented into it and it is possible that it adds a fee in the future.Code Snippet
https://github.com/sherlock-audit/2022-11-float-capital/blob/main/contracts/market/template/MarketCore.sol#L293
Tool used
Manual Review
Recommendation
For fee-on-transfer tokens, it is best to check the balance before and after the transfer, to see the actually received value. For rebasing tokens it's best to add an
allowlistfunctionality for ERC20 tokens.Duplicate of #10