Contract interfaces allow nonpayable implementations of payable functions

Severity

Medium Risk

Relevant GitHub Links

https://github.com/vyperlang/vyper/blob/3ba14124602b673d45b86bae7ff90a01d782acb5/vyper/semantics/types/user.py#L316-L331

Summary

When a contract interface is implemented, the compiler checks that each function in the interface has a corresponding public function in the contract. However, it does not check that the functions have the same visibility, which can lead to dangerous situations.

Vulnerability Details

When performing semantic analysis on a contract that implements an interface, the compiler calls type_.validate_implements(node) to confirm that the interface is correctly implemented.

This function iterates through all public functions on the interface, checks that we have implemented a function with the same name, and then verifies that all the arguments and return types are of the same type. Finally, it checks that the state mutability of our function is not greater than the interface.

def implements(self, other: "ContractFunctionT") -> bool:
    """
    Checks if this function implements the signature of another
    function.

    Used when determining if an interface has been implemented. This method
    should not be directly implemented by any inherited classes.
    """

    if not self.is_external:
        return False

    arguments, return_type = self._iface_sig
    other_arguments, other_return_type = other._iface_sig

    if len(arguments) != len(other_arguments):
        return False
    for atyp, btyp in zip(arguments, other_arguments):
        if not atyp.compare_type(btyp):
            return False

    if return_type and not return_type.compare_type(other_return_type):  # type: ignore
        return False

    if self.mutability > other.mutability:
        return False

    return True

If we look at the mutability enum, we can see that "greater than" represents a less restrictive mutability:

class StateMutability(_StringEnum):
    PURE = _StringEnum.auto()
    VIEW = _StringEnum.auto()
    NONPAYABLE = _StringEnum.auto()
    PAYABLE = _StringEnum.auto()

This means that, although we cannot take a view function on the interface and implement it as a nonpayable function, we can do the inverse and implement any function as a more restrictive type.

While for some types this may make sense, it can lead to problems with payable functions.

Interfaces are intended to define the behavior that is required for a contract to perform. If an interface defines a function as payable, it is safe for interacting contracts to send ETH to that function. However, if a contract that implements that interface changes that function to nonpayable (or to view), it could cause the interacting contracts to revert.

Impact

Contracts that Vyper considers to be correctly implementing an interface may not reflect the expectations of the interface, and interacting contracts may end up locked because they expect to be able to send ETH to a function that is not payable.

Note that Solidity has a similar check that "lower" mutabilities are acceptable when implementing an interface, but has a specific carveout for payable functions to avoid this risk. See the table below for a breakdown of the similarities and differences.

------------------------- Solidity ------------ Vyper view => nonpayable NO NO ✓ view => payable NO NO ✓ nonpayable => view/getter YES YES ✓ nonpayable => payable NO NO ✓ payable => view/getter NO YES <== this is the issue payable => nonpayable NO YES <== this is the issue

Tools Used

Manual Review

Recommendations

In the implements() function, check whether the mutability of the function on the interface is payable. If it is, require the implementing contract to make the function payable as well.