IAction.rolloverPosition and IAction.closePosition are not always called together as part round lifecycle

[andreiashu]

Description

When a round is started, the Vault owner will call rollOver function, which in turn calls _distribute. The IAction.rolloverPosition is called only if the percentage allocation amount is non-zero:

https://github.com/monoceros-alpha/review-opyn-perp-vault-templates-2021-07/blob/d94fcb2e2173008272604705a9fc618710349462/code/contracts/core/OpynPerpVault.sol#L447-L450

When a round is closed, the IAction.closePosition function is always called:

https://github.com/monoceros-alpha/review-opyn-perp-vault-templates-2021-07/blob/d94fcb2e2173008272604705a9fc618710349462/code/contracts/core/OpynPerpVault.sol#L419-L422

Upon rollover round, the state in the action is updated:

https://github.com/monoceros-alpha/review-opyn-perp-vault-templates-2021-07/blob/d94fcb2e2173008272604705a9fc618710349462/code/contracts/utils/RollOverBase.sol#L64-L70

And the close position step is dependent on this state in order to correctly function:

https://github.com/monoceros-alpha/review-opyn-perp-vault-templates-2021-07/blob/d94fcb2e2173008272604705a9fc618710349462/code/contracts/example-actions/ShortOToken.sol#L234-L238

Recommendation

While we have yet to find a specific issue with the code under review, we believe that as a design decision, the system should always call IAction.rolloverPosition and IAction.closePosition in tandem.

In the case above, either always call IAction.rolloverPosition regardless of the allocation percentage for the action or add code in _closeAndWithdraw function to only call IAction.closePosition if the action's balance is non-zero.