Cally contest - peritoflores's results

Lines of code

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

Vulnerability details

Impact

Malicious owner can steal all ETH of a sell.

Proof of Concept

The function setFee#CallyNFT.sol is critical as it set the amount of ETH that the protocol will receive. A malicious owner can set the fee to 1e18 and all ETH after exercise will go to the owner of the contract.

if (feeRate > 0) {                          #CallyNFT.sol#L288

 fee = (msg.value * feeRate) / 1e18;

  protocolUnclaimedFees += fee;

}

The seller (or beneficiary) will receive 0 ETH.

Recommended Mitigation Steps

In addition to the emitted event I would introduce a maximum fee to add transparency.

Ideally every time the fee changes it should only affect the new auction but not the previous. For example with a timelock. Although this requires much more work, maybe an idea could be to put fee as a parameter of the vault which is completed with the current fee.

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#L431-L453

Vulnerability details

Impact

Buyers can accidentally lose their NFT if they send to incorrect address.

Proof of Concept

When the buyer decide to call exercise the NFT is transfered using transferFrom. This is risky because if the destination (msg.sender) is a contract and it is unable to handle NFT then it will be locked forever. This function in my opinion is Ok when you transfer to your contract.

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

Similar issues

https://github.com/code-423n4/2022-04-backed-findings/issues/83

Recommended Mitigation Steps

Implement safeTransferFrom