Current implementation of isValidSignatureNow will be broken due to EIP7377

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-02-uniswap-foundation/blob/main/src/UniStaker.sol#L803-L809

Vulnerability details

Preface

It's important to note that these two areas were important for us participants to explore, as per README.md:

* Issues caused by changes in network conditions due to network upgrade (the contracts are intended to be immutable)
* Attacks or bugs caused by account abstraction or smart contract wallet usage

This is important to keep in mind for our report.

Description

Inside UniStaker.sol, the _revertIfSignatureIsNotValidNow function gets invoked in different places:

  function _revertIfSignatureIsNotValidNow(address _signer, bytes32 _hash, bytes memory _signature)
    internal
    view
  {
    bool _isValid = SignatureChecker.isValidSignatureNow(_signer, _hash, _signature);
    if (!_isValid) revert UniStaker__InvalidSignature();
  }

  // _revertIfSignatureIsNotValidNow is called inside these functions
  function stakeOnBehalf
  function stakeMoreOnBehalf
  function alterDelegateeOnBehalf
  function alterBeneficiaryOnBehalf
  function withdrawOnBehalf
  function claimRewardOnBehalf

Inside of _revertIfSignatureIsNotValidNow, the isValidSignatureNow function is called:

    function isValidSignatureNow(address signer, bytes32 hash, bytes memory signature) internal view returns (bool) {
        (address recovered, ECDSA.RecoverError error, ) = ECDSA.tryRecover(hash, signature);
        return
            (error == ECDSA.RecoverError.NoError && recovered == signer) ||
            isValidERC1271SignatureNow(signer, hash, signature);
    }

However, at the moment of writing, there is a problem with this function which, in the light of these contracts being immutable, will pose a problem due to network upgrades.

When EIP7377 gets implemented, deployers will have a private key for a contract because it would be possible to deploy code at its address. The current implementation of isValidSignatureNow assumes that every contract address interacting with the function has no known private key.

This issue has been notified here by an OpenZeppelin Engineer and a current mitigation has been proposed. Due to the fact that the Sponsor was interested in findings regarding changes in network conditions and bugs caused by AA, we are submitting this report.

Tools Used

Manual Review

Recommended Mitigation Steps

Use the recommendation by one of the Openzeppelin Engineers:

function isValidSignatureNow(address signer, bytes32 hash, bytes memory signature) internal view returns (bool) {
    if (signer.code.length == 0) {
        (address recovered, ECDSA.RecoverError error, ) = ECDSA.tryRecover(hash, signature);
        return (error == ECDSA.RecoverError.NoError && recovered == signer);
    } else {
        return isValidERC1271SignatureNow(signer, hash, signature);
    }
}

Assessed type

Other

[c4-sponsor]

apbendi (sponsor) disputed

[apbendi]

We appreciate this report and find it useful as an informational issue, but we do not consider it valid as a vulnerability. The EIP in question (7377) is far from finalized. If adopted as-is, the impacts on the whole ecosystem would be massive. All permit tokens, for example, (including the UNI token itself!) would be vulnerable to the issue described here. For this reason, we find it highly unlikely that 7377 will be adopted without some change to the behavior of the ecrecover opcode. Such a change has already been proposed, namely that EOAs with code deployed would no longer be controllable by the EOA private key. For this reason, we don't consider this an issue, and we don't consider the mitigation recommended as worth the gas overhead that would be realized.

[MarioPoneder]

This is an interesting report and I value the due diligence. However, I cannot justify Medium severity due to speculation on the unlikely future that the EIP will be finalized as it is and therefore leading to the reported/discussed issue.

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

MarioPoneder marked the issue as grade-b