Use Address.sendValue instead of address.tranfer in RefundEscrow

[Amxx]

using address.tranfer is bad practice. This avoid issues if _beneficiary is a smart contract.

[frangio]

Well this is a good moment to discuss, why do you prefer this syntax for using libraries?

[Amxx]

Because it avoid any ambiguity over the fact that this is a library being called, and not a builtin function. Imagine if Address contained a transfer function (good thing it doesn't) ... then what would address.transfer be ? The library, the built-in function ?

I think the using for syntax is great for safemath. I also think it is acceptable when the first type is clearly not a contract (EnumerableMap/EnumerableSet) ... For addresses I'm not sure ... for contracts I think it's a terrible idea.

In TokenTimelock there is an IERC20 token that is called using _token.safeTransfer(beneficiary(), amount);. I think there is a real ambiguity between a potential safeTransfer function included in IERC20, and a safeTransfer library function that is used for IERC20 (which is the case here).

TLDR:
In the particular case, I wouldn't oppose doing _beneficiary.sendValue(address(this).balance);, but there are cases where I think this syntax is to be avoided.

[Amxx]

I noticed that using XXX for address; and using XXX for address payable; are two separate things.

pragma solidity ^0.8.0;

library Address {
    event Sent(address,uint256);
    function transfer(address to, uint256 value) internal {
        (bool success, ) = to.call{value: value}("");
        require(success);
        emit Sent(to, value);
    }
}

contract Test {
    using Address for address;
    receive() external payable {}
    
    function send1(address payable to, uint256 value) external payable {
        to.transfer(value); // built in mechanism
    }
    
    function send2(address to, uint256 value) external payable {
        to.transfer(value); // using Library
    }
}

EDIT: in the code above, if we replace using Address for address; with using Address for address payable;, send1 compilation gives an error (member not unique).

[frangio]

They're good points. I agree that for contracts it's confusing. For this case I would leave the using for syntax for consistency with the rest of the library.