Cally contest - 0x1337's results

Lines of code

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

Vulnerability details

Impact

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.

Proof of Concept

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.

        vault.tokenType == TokenType.ERC721
            ? ERC721(vault.token).transferFrom(msg.sender, address(this), vault.tokenIdOrAmount)
            : ERC20(vault.token).safeTransferFrom(msg.sender, address(this), vault.tokenIdOrAmount);

The implication is that both the exercise() function and the withdraw() function are guaranteed to revert if there's 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.

Tools Used

Manual review

Recommended Mitigation Steps

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.

There's a typo in line 169. The text in the require statement should be "Reserve strike too big" instead of "Reserve strike too small"

require(dutchAuctionReserveStrike < strikeOptions[dutchAuctionStartingStrikeIndex], "Reserve strike too small");