mstpr-brainbot - Fee on transfer tokens will break the withdrawing process

[sherlock-admin]

mstpr-brainbot

high

Fee on transfer tokens will break the withdrawing process

Summary

If a currency has a built-in transfer fee, withdrawing prime cash may be impossible due to accounting discrepancies.

Vulnerability Detail

Example: Alice has 100 pUSDT, equivalent to 105 USDT, and assume that all the underlying USDT is in Compound V3 (in form of cUSDT), earning interest.

When Alice withdraws the prime cash using the withdraw() function in AccountsAction.sol, the function checks if the corresponding underlying (105 USDT) is available in the contract. Since all the USDT is lent out in Compound, Notional initiates the redemption process. The redemption process attempts to withdraw 105 USDT worth of cUSDT from Compound. However, due to transfer fees on USDT, redeeming 105 USDT worth of cUSDT results in approximately 104.9 USDT. The require check ensures that Notional must withdraw 105 USDT or more, but in reality, only 104.9 USDT is withdrawn, causing the function to revert consistently.

Impact

Since this is an unlikely scenario I'll label it as medium.

However, if fee on transfer tokens will be used this can be a high finding since withdrawals will not go through at all. USDT can open it's transfer functionality so that should be also taken into consideration if such thing happens.

Code Snippet

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/external/actions/AccountAction.sol#L173

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/internal/balances/TokenHandler.sol#L220-L247

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/internal/balances/TokenHandler.sol#L249-L278

revert lines
https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/internal/balances/TokenHandler.sol#L383

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/internal/balances/TokenHandler.sol#L277

Tool used

Manual Review

Recommendation

Instead of promising the underlying amount on withdrawals, just return the withdrawn pcashs corresponding yield tokens underlying amount and let users endorse the loss

[jeffywu]

This is somewhat true, although if this really happened and we needed to manage it the PrimeCashHoldingsOracle could return a lower external balance to account for the transfer fee.

[Jiaren-tang]

Escalate for 10 USDC. fee on transfer is not in scope and this issue should not be a seperate medium

the onchain context is:

DEPLOYMENT: Currently Mainnet, considering Arbitrum and Optimisim in the near future.
ERC20: Any Non-Rebasing token. ex. USDC, DAI, USDT (future), wstETH, WETH, WBTC, FRAX, CRV, etc.
ERC721: None
ERC777: None
FEE-ON-TRANSFER: None planned, some support for fee on transfer

clearly none of the supported ERC20 token is fee-on-transfer token

and the protocol clearly indicate

FEE-ON-TRANSFER: None planned, some support for fee on transfer

[sherlock-admin]

Escalate for 10 USDC. fee on transfer is not in scope and this issue should not be a seperate medium

the onchain context is:

DEPLOYMENT: Currently Mainnet, considering Arbitrum and Optimisim in the near future.
ERC20: Any Non-Rebasing token. ex. USDC, DAI, USDT (future), wstETH, WETH, WBTC, FRAX, CRV, etc.
ERC721: None
ERC777: None
FEE-ON-TRANSFER: None planned, some support for fee on transfer

clearly none of the supported ERC20 token is fee-on-transfer token

and the protocol clearly indicate

FEE-ON-TRANSFER: None planned, some support for fee on transfer

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Result:
Medium
Unique
Considering this a valid issue as the readme indicates support of Fee-on-Transfer token is intended.

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• ShadowForce: rejected