Cally contest - minhquanym's results

Judge has assessed an item in Issue #95 as Medium risk. The relevant finding follows:

Incompatability with deflationary / fee-on-transfer tokens Function Cally.createVault function takes a tokenIdOrAmount parameter but this parameter is not the actual transferred amount for fee-on-transfer / deflationary (or other rebasing) tokens in case tokenType = ERC20 Impact The actual deposited amount might be lower than the specified depositAmount of the function parameter. And when users exercise or withdraw they not only receive less than expected amount but also take funds of other vaults with the same vault.token too, causes loss of funds. Proof-of-concept https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L200 https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L296 https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L345 Recommended Mitigation Steps Transfer the tokens first and compare pre-/after token balances to compute the actual amount.

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L295 https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L344

Vulnerability details

Impact

• In the function Cally.exercise() and Cally.withdraw(), ERC721 token is sent to msg.sender and the transferFrom keyword is used instead of safeTransferFrom. If msg.sender is a contract and is not aware of incoming ERC721 tokens, the sent tokens could be locked forever

Proof of Concept

• https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L295
• https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L344

Recommended Mitigation Steps

• Consider changing transferFrom to safeTransferFrom.

1. Incompatability with deflationary / fee-on-transfer tokens

• Function Cally.createVault function takes a tokenIdOrAmount parameter but this parameter is not the actual transferred amount for fee-on-transfer / deflationary (or other rebasing) tokens in case tokenType = ERC20

Impact

• The actual deposited amount might be lower than the specified depositAmount of the function parameter.
• And when users exercise or withdraw they not only receive less than expected amount but also take funds of other vaults with the same vault.token too, causes loss of funds.

Proof-of-concept

• https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L200
• https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L296
• https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L345

Recommended Mitigation Steps

• Transfer the tokens first and compare pre-/after token balances to compute the actual amount.

2. Vault id will start from 3 not from 1.

• In Cally.createVault() function, vaultIndex += 2 is done before assigning it to vaultId. So vaultId will start from 3 and optionId will start from 4. There will not have tokenId = 0 or tokenId = 1
• This is okay, but it’s kind of abnormal. And it’s just a QA issue.

Proof of concept

• https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L188-L189

Recommended Mitigation Steps

• Assign vaultIndex to vaultId before add 2.

vaultId = vaultIndex;
vaultIndex += 2;

1. Use storage when read only 1 field from a struct can save gas

• In Cally.getPremium function, we only need 1 field from vault which is vault.premiumIndex but a whole Vault is loaded to memory.

• We can save gas by using storage instead of memory

Proof of concept

• https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L395

Recommended Mitigation Steps

• Use storage instead of memory in case you not need all the struct data.

2. Write to storage directly instead of modifying a copy of its in memory then write the whole in storage can save gas.

• In Cally.buyOption and Cally.exercise function, we only modify one or two data fields of vault but we write a whole data struct to storage, including non-changed data.

• And since write to storage is one of the most expensive operation. We should save gas by only writing changed data to storage.

Proof of concept

• https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L231-L241
• https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L278-L279

Recommended Mitigation Steps

• Write directly to storage instead. For example,

_vaults[vaultId].isExercised = true;