Cally contest - reassor's results

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L119-L121 https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L283-L286

Vulnerability details

Impact

Contract Cally allows exercising a call option by sending the underlying assets to the exerciser and the strike ETH to the vault's beneficiary. Contract can charge a feeRate applied during exercising. The issue is that the feeRate can be dynamically changed by the administrator at any time and the change is applied also to already existing vaults. That means that administrator can launch an attack where he changes the feeRate to 100% and then during exercising all funds are transferred to him as a fee.

Exploit Scenario:

1. User(s) can see that the feeRate is 0% so he creates the vault via createVault.
2. Other user(s) starts buying options.
3. Administrators increases feeRate to 100% via setFee (or runs front-running attack).
4. setFee transaction is being included before user's exercise transaction.
5. Administrator steals all the ethers that were expected to be received by vault's beneficiary.

This issue is also relevant in case of administrator not being a malicious actor. User accepts some kind of fee that is visible as a feeRate for example 1%. Then administrator just change the feeRate to 3%. User didn't expect that change and effectively loses funds.

Proof of Concept

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to assign current feeRate to vault's vault.feeRate in createVault. During exercise only the vault.feeRate should be used as a feeRate. In addition it is recommended to add timelock for changing feeRate via setFee so it will be not possible to launch front-running attack.

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L174 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

Vulnerability details

Impact

Contract Cally does not properly handle ERC20 tokens that charge fee on their transfers. Implementation of such a tokens does not transfer exact amount provided to transfer() but part of it is charged as a fee, burned or used in some other way. This leads to incorrect accounting and effectively to loss of funds.

Proof of Concept

• https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L174
• 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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add support for ERC20 tokens with built-in fees. Example of the implementation:

uint256 startingBalance = IERC20(token).balanceOf(address(this));
IERC20(token).safeTransferFrom(msg.sender, address(this), amount);
uint256 endingBalance = IERC20(token).balanceOf(address(this));

vault.tokenIdOrAmount = endingBalance - startingBalance;

1. Missing zero address checks

Risk

Low

Impact

Contract Cally does not check for zero addresses which might lead to loss of funds, failed transactions and can break the protocol functionality.

Proof of Concept

• Missing check for token address - https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L160
• Missing check for beneficiary address - https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L351

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add zero address checks for listed parameters.

2. Missing events

Risk

Low

Impact

Contract Cally does not implement events for some critical functions. Lack of events makes it difficult for off-chain applications to monitor the protocol.

Proof of Concept

• Missing setFee event - https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L119-L121
• Missing withdrawProtocolFees event - https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L124-L128
• Missing setVaultBeneficiary event - https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L351-L357

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add missing events to listed functions.

3. Missing validation for feeRate

Risk

Low

Impact

Function Cally.setFee is missing check for feeRate_ parameter that should not exceed acceptable by protocol percentage.

Proof of Concept

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add check for feeRate_ parameter.

4. Owner - critical address change

Risk

Low

Impact

Changing critical addresses such as ownership should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one. This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

Proof of Concept

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to implement two-step process for passing ownership.

5. Usage of boolean values

Risk

Non-Critical

Impact

Contract uses false boolean expression for require statements in multiple functions.

Proof of Concept

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to remove false expression and use alternative syntax such as:

require(!vault.isExercised, "Vault already exercised");

6. Incorrect error message

Risk

Non-Critical

Impact

Function Cally.createVault checks if dutchAuctionReserveStrike is smaller than specified strike option. Incorrect error message "Reserve strike too small" is returned in case of the error. It should be "Reserve strike too big".

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

Proof of Concept

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to change the error message to "Reserve strike too big".