Registering a deposit uses the incorrect initial value for the user

[cleanunicorn]

Description

When the vault is locked, the users can still queue deposits and withdraws. However, they cannot be immediately accessed, so specific accounting needs to be done.

Users call registerDeposit:

https://github.com/monoceros-alpha/review-opyn-perp-vault-templates-2021-07/blob/3d44603300dd9abffe5a1c1e1c2647e9f6b80c7b/code/contracts/core/OpynPerpVault.sol#L245

The funds they wish to deposit are moved into the vault.

https://github.com/monoceros-alpha/review-opyn-perp-vault-templates-2021-07/blob/3d44603300dd9abffe5a1c1e1c2647e9f6b80c7b/code/contracts/core/OpynPerpVault.sol#L246

And the accounting needs to be done for this user to be handled later.

https://github.com/monoceros-alpha/review-opyn-perp-vault-templates-2021-07/blob/3d44603300dd9abffe5a1c1e1c2647e9f6b80c7b/code/contracts/core/OpynPerpVault.sol#L249-L251

On top of the initial value of userRoundQueuedDepositAmount[_shareRecipient][round], we need to add the current deposited value of _amount.

However, a copy/paste artifact from registerWithdraw was left in the code, the initial value used is userRoundQueuedWithdrawShares[_shareRecipient][round] instead of userRoundQueuedDepositAmount[_shareRecipient][round].

Recommendation

Update the accounting to reflect the correct initial value of the queued deposit.

Observation

These kinds of bugs should be caught by the tests.

The test that checks this functionality being right is located here.

https://github.com/monoceros-alpha/review-opyn-perp-vault-templates-2021-07/blob/67ce34e00f9c9aa0036573e1c5e144c5f6cffd70/code/test/unit-tests/core/OpynPerpVault.ts#L318-L338

However, the functionality isn't completely tested. This test only checks going from 0 to depositAmount. The broken functionality is related to increasing the deposit amount, which is done by this piece of code (where the bug exists):

https://github.com/monoceros-alpha/review-opyn-perp-vault-templates-2021-07/blob/67ce34e00f9c9aa0036573e1c5e144c5f6cffd70/code/contracts/core/OpynPerpVault.sol#L249-L251

This hints at the way the code was developed. A test-driven development is a good way to have well-tested code, but the tests need to be written before the code is implemented, and the code needs to only satisfy the tests. In this case, there is additional functionality (increasing the deposit size) that doesn't have an associated test.

Not having a specific test for additional functionality, added to the fact that there are tests checking the code, gives a false sense of security about the correctness of the code.