Cally contest - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

In case excess ETH is sent while calling buyOption, then that excess ETH is never refunded back to user and instead added to the balance of beneficiary

Proof of Concept

1. User A calls buyOption function with msg.value as 10
2. The premium is calculated as 6
3. Ideally User A must be refunded 10-6=4 back but instead whole msg.value 10 is added to balance of beneficiary

Recommended Mitigation Steps

If user has mistakenly sent msg.value>premium then msg.value-premium should be refunded back to the user

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

setFee function : Add a check to see fees is in correct range:

require(feeRate_>0 && feeRate_<=100, "Too High/Low Fees")

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

createVault function: This function does not allow to set beneficiary. This becomes a problem if user creates a vault and someone immediately calls buyOption on this vault before has chance to set beneficiary

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

tokenURI function: If isVaultToken is false then Vault is using id as tokenId - 1 even though the options for Vault are placed at tokenId + 1. Change Cally.sol#L459 to

Vault memory vault = _vaults[isVaultToken ? tokenId : tokenId + 1];