SmartEscrow Audit Feedback

[stevieraykatz]

This PR incorporates feedback from the second smart contract escrow audit conducted by spearbit:

1. will do
2. I think that the clearer error messaging on construction is more helpful than the minimal gas removing the check saves. Won't do
3. Same reasoning as 2). Won't do
4. See PR comments below for context; ultimately; No change needed.

--- FROM SPEARBIT ---

1. Redundant contractTerminated check, will be checked in release()
   https://github.com/base-org/contracts/blob/b49add823005826521b5b8d8cf8a7a292420f0bd/src/smart-escrow/SmartEscrow.sol#L186

2. https://github.com/base-org/contracts/blob/b49add823005826521b5b8d8cf8a7a292420f0bd/src/smart-escrow/SmartEscrow.sol#L155-L157
   if (_start >= _end) revert StartTimeAfterEndTime(_start, _end);
   Technically redundant as it will be covered by the checks:
   if (_cliffStart < _start) and if (_cliffStart >= _end)
   which enforces _start <= _cliffStart < end

3. https://github.com/base-org/contracts/blob/b49add823005826521b5b8d8cf8a7a292420f0bd/src/smart-escrow/SmartEscrow.sol#L160-L165
   Likewise, the check below is technically redundant
   if ((_end - _start) < _vestingPeriodSeconds) {
   revert VestingPeriodExceedsContractDuration(_vestingPeriodSeconds);
   }
   because it will be covered by this check:
   if ((_end - _start) % _vestingPeriodSeconds != 0)

4. Wanna confirm the expected behaviour for access control and benefactor / beneficiary address changes.
   The updateBenefactor() / updateBeneficiary() can only be called by BENEFACTOR_OWNER_ROLE / BENEFICIARY_OWNER_ROLE respectively, but this strictly updates the benefactor / beneficiary variables. Access control updates: revoking the OWNER and TERMINATOR roles of the old address, and granting them to the new one, is handled by the public grantRole() / revokeRole() / renounceRole() methods. (edited)

[HickupHH3]

For (4), the concern I have is operational: when there's a change to BENEFICIARY / BENEFACTOR by either party, might miss revoking the old and / or granting the new either the BENEFACTOR_OWNER_ROLE / BENEFICIARY_OWNER_ROLE and TERMINATOR roles. Also, it takes another step from the multisig to execute, so there's a bit of a delay.

[stevieraykatz]

For (4), the concern I have is operational: when there's a change to BENEFICIARY / BENEFACTOR by either party, might miss revoking the old and / or granting the new either the BENEFACTOR_OWNER_ROLE / BENEFICIARY_OWNER_ROLE and TERMINATOR roles. Also, it takes another step from the multisig to execute, so there's a bit of a delay.

I see your concern. Good callout. I'll add a comment for these methods specifically calling out the 2-tx flow necessary to using these methods correctly.

I think that we will live with the known operational complexity to avoid adding contract-change scope at this point.