Submitted by 0x1337, also found by 0x52, 0xDjango, 0xsanson, berndartmueller, BondiPestControl, BowTiedWardens, cccz, dipp, GimelSec, hake, hickuphh3, horsefacts, hubble, IllIllI, MaratCerby, MiloTruck, minhquanym, PPrieditis, reassor, shenwilly, smilingheretic, TrungOre, VAD37, and WatchPug_
https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L198-L200
https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L294-L296
https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L343-L345
Some ERC20 tokens charge a transaction fee for every transfer (used to encourage staking, add to liquidity pool, pay a fee to contract owner, etc.). If any such token is used in the createVault() function, either the token cannot be withdrawn from the contract (due to insufficient token balance), or it could be exploited by other such token holders and the Cally contract would lose economic value and some users would be unable to withdraw the underlying asset.
Plenty of ERC20 tokens charge a fee for every transfer (e.g. Safemoon and its forks), in which the amount of token received is less than the amount being sent. When a fee token is used as the token in the createVault() function, the amount received by the contract would be less than the amount being sent. To be more precise, the increase in the cally contract token balance would be less than vault.tokenIdOrAmount for such ERC20 token because of the fee.
The implication is that both the exercise() function and the withdraw() function are guaranteed to revert if theres no other vault in the contract that contains the same fee tokens, due to insufficient token balance in the Cally contract.
When an attacker observes that a vault is being created that contains such fee tokens, the attacker could create a new vault himself that contains the same token, and then withdraw the same amount. Essentially the Cally contract would be paying the transfer fee for the attacker because of how the token amount is recorded. This causes loss of user fund and loss of value from the Cally contract. It would make economic sense for the attacker when the fee charged by the token accrue to the attacker. The attacker would essentially use the Cally contract as a conduit to generate fee income.
Recommend disallowing fee tokens from being used in the vault. This can be done by adding a require() statement to check that the amount increase of the token balance in the Cally contract is equal to the amount being sent by the caller of the createVault() function.
outdoteth (Cally) confirmed and commented:
HardlyDifficult (judge) decreased severity to Medium and commented:
