_cancelAndRefund is not protected from re-entrancy

[code423n4]

Handle

pauliax

Vulnerability details

Impact

function _cancelAndRefund is not protected from re-entrancy. _cancelAndRefund uses an unsafe _transfer that performs a low-level call which is susceptible to re-entrancy attack. These calls should be the last step or you may receive unexpected guests. Even the comment says that but it is actually not the last step:

    if (refund > 0) {
      // Security: doing this last to avoid re-entrancy concerns
      _transfer(tokenAddress, _keyOwner, refund);
    }
    // inform the hook if there is one registered
    if(address(onKeyCancelHook) != address(0))
    {
      onKeyCancelHook.onKeyCancel(msg.sender, _keyOwner, refund);
    }

Recommended Mitigation Steps

Consider either making this _transfer the last step or adding re-entrancy protection: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol

[julien51]

Interesting. Considering here that the hook itself can only be set by a lock manager, I would assume that it would not trigger a re-entrency call to canceling a key again. Additionally the function _cancelAndRefund can only be called if the the key owner has a valid key (hasValidKey) and the key is expired before the refund.

so all in all, i think we should clarify the comment but I would think we are actually safe here.

[0xleastwood]

Agree with sponsor. Doesn't seem like its exploitable but will keep as low.