search

code-423n4 / 2022-05-cally-findings

2 stars 0 forks source link

`TokenType` mismatch with true type can be exploited to steal funds #309

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L197-L200

Vulnerability details

A vault can be created for different TokenTypes, namely ERC20 and ERC721. These tokens have different logic, so a user needs to pass the type of the underlying token when creating a vault. At the end of createVault the user's tokens are pulled into the contract (L197):

// transfer the NFTs or ERC20s to the contract
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 issue is that an user can choose to mismatch vault.tokenType and the true vault.token's type.

More importantly, if they use tokenType=ERC721 for a ERC20 that doesn't revert on failure (but returns false for example), they can basically create a vault for free.

Proof of concept

This situation causes two attack vectors:

(1) Alice creates a vault as explained before with a premium and a very favourable strike for the buyer. Bob is monitoring vaults' creations by checking the pair (vault.token, vault.tokenIdOrAmount); he immediately buyOption thinking he made a good deal, but during exercise he doesn't get any token. Alice gained premium + strike for free. (2) Let's assume there are already some ERC20 tokens that don't revert on transferFrom in the contract. Alice can create a vault with vault.tokenIdOrAmount equal the balance of the contract; she then can call initiateWithdraw+withdraw, and the ERC721(vault.token).transferFrom(address(this), msg.sender, vault.tokenIdOrAmount) will actually succeed. Basically she drained the contract of all these tokens.

Recommendations

A possible fix may consist in calling vault.token querying for EIP165's supportsInterface(bytes4 interfaceId) with interfaceId = 0x80ac58cd (ERC721 interfaceId). If the staticall fails or returns false then the token is ERC20, otherwise ERC721.

Another fix can be separating Cally into two contracts, one for ERC20 and the other for ERC721.

outdoteth commented 2 years ago

no-revert-on-transfer tokens can be drained: https://github.com/code-423n4/2022-05-cally-findings/issues/89