code423n4 commented 2 years ago

Handle

0xsanson

Vulnerability details

Impact

The current implementation doesn't work with fee-on-transfer underlying tokens. Considering that Compound can have these kind of tokens (ex. USDT can activate fees), this issue can affect the protocol.

The problem arise when transferring tokens, basically blocking all functions in Swivel.sol for that particular token, since the contract wrongly assumes balances values. This becomes particularly problematic in the following scenario: a market for USDT is running without problems, then they activate the fee: this effectively blocks users from redeeming the underlying.

Proof of Concept

grep 'transfer' Swivel.sol for a complete list of affected lines (basically every tranfer or transferFrom of underlying tokens). Also grep 'redeemUnderlying' Swivel.sol.

For example:

require(CErc20(mPlace.cTokenAddress(u, m)).redeemUnderlying(redeemed) == 0, 'compound redemption failed');
// transfer underlying back to msg.sender
Erc20(u).transfer(msg.sender, redeemed);

This would fail (revert) since the contract would have received less than redeemed tokens.

Tools Used

editor

Recommended Mitigation Steps

If the protocol wants to use all possible Compund's tokens, a way to handle these tokens must be implemented. A possible way to do it is to check the balance of the contract before and after every time a token is transferred to see the effective quantity. To help keeping the code clear, a function like Compund's doTransferIn can be implemented.

JTraversa commented 2 years ago

Will review further. I dont believe that any tokens on compound currently have fees. Although it is news to me that USDT has toggle-able fee's woops.

That said, given we have admin control over added assets, I'd probably also lower this to a low-risk if accepted.

0xean commented 2 years ago

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

based on this "leaking value" I would say it qualifies as a med-severity.

JTraversa commented 2 years ago

We can account for the transfers in with a similar balance before transferFrom, and balance after check, in order to prevent additional deposits after a fee has been turned on.

That said, Im not sure we can account for redeemUnderlying easily because should a fee be turned on, all funds would just be stuck in our contract if we added a similar check? (a != balance2-balance1)

If a fee is turned on for USDT markets, there would be lost fee value, so if adding a check wouldn't work, the most reasonable response is just to make sure the market is pausable.

code-423n4 / 2021-09-swivel-findings