code423n4 commented 2 years ago

Handle

0x1f8b

Vulnerability details

Impact

Logic mismatch that would confuse users or produce unexpected errors.

Proof of Concept

In the ConvexModule contract the logic of the addVault method contemplates that if the user use vaultId=0, this Id will be replaced by cachedVaultId, however during the elimination logic in the removeVault method this conditional is not present. Therefore, the user who tries to delete the Id 0 will surely get an error, the scope of this issue is unknown as part of the code is out of scope..

Tools Used

Manual review.

Recommended Mitigation Steps

Use the same logic for remove and add or return the added Id in addVault method.

iamsahu commented 2 years ago

The issue could be mitigated by adding comments explaining the logic behind Id being replaced by cachedVaultId

alcueca commented 2 years ago

vaultId = 0 does mean the cachedVaultId.

It sounds weird to remove a vault from the wrapper in the same transaction that the vault is built, but the correct behaviour would be to accept it.

GalloDaSballo commented 2 years ago

Same as #45

@alcueca Was this finding useful and did you implement as suggested by the PR?