search

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

2 stars 0 forks source link

`exercise()` Does Not Burn a Vault's NFT #205

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#L258-L297

Vulnerability details

Impact

The exercise() function is called by an option holder prior to expiration to pay the agreed upon strike price and in return, receive the NFT. While the caller's option is burnt by the protocol, the vault NFT is not. This isn't overly exploitable, but it leads to some interesting behaviour where a vault owner can call initiateWithdraw(), setVaultBeneficiary(), tokenURI() and transferFrom(). It would be safer to ensure that these functions are not callable by burning the vault's NFT. This should limit the total attack surface open to the attacker.

Recommended Mitigation Steps

Consider burning a vault's NFT whenever an option is exercised by its holder.

outdoteth commented 2 years ago

it's intentional that the vault is not burned this is so that on the frontend we can still show a user their vaults and which ones have been exercised or not if we burn the nft, the user no longer owns the vault and it wont show up on the ui in the "your vaults" page; https://rinkeby.cally.finance/

no potential exploit is given here so disagree with severity.

HardlyDifficult commented 2 years ago

This seems to be by design & details about a risk were not provided.