Signature Replay Vulnerability#

Vulnerability#

In ERC721Wrapper.sol wrapBySig and unwrapBySig functions, a nonce is passed in both function arguments, but the nonce's uniqueness is never enforced in the function logic. This leaves these functions open to signature replay attacks in which the original signature is used again at a later time against end user wishes (given the signature expiry is high enough).

Code#

function wrapBySig(
    uint256 tokenId,
    uint256 nonce, // passed here
    uint256 expiry,
    uint8 v,
    bytes32 r,
    bytes32 s
) external {
    require(
        block.timestamp <= expiry,
        "ERC721Wrapper::wrapBySig: signature expired"
    );

    // included in the struct hash
    bytes32 structHash = keccak256(
        abi.encode(_WRAP_TYPEHASH, tokenId, nonce, expiry)
    );
    bytes32 digest = keccak256(
        abi.encodePacked("\x19\x01", _domainSeparator(), structHash)
    );
    address signer = ecrecover(digest, v, r, s);

    require(
        signer != address(0),
        "ERC721Wrapper::wrapBySig: invalid signature"
    );

    // Never verified

    _wrap(signer, tokenId);
    _onTokenWrap(signer, 1);
}

Test Case#

it('is able to be replayed (test passing is undesirable)', async function () {
  await testNFT.mint(accounts[0].address, 1);
  await testNFT.mint(accounts[0].address, 2);
  await testNFT.setApprovalForAll(erc721WrapperVotes.address, true);

  const { chainId } = await ethers.provider.getNetwork();
  const sig0 = await accounts[0]._signTypedData(
    {
      name: 'Wrapped Test Token',
      chainId,
      verifyingContract: erc721WrapperVotes.address,
    },
    {
      Wrap: [
        { name: 'tokenId', type: 'uint256' },
        { name: 'nonce', type: 'uint256' },
        { name: 'expiry', type: 'uint256' },
      ],
    },
    { tokenId: 1, nonce: 1, expiry: 10e9 }
  );

  const r0 = '0x' + sig0.substring(2, 66);
  const s0 = '0x' + sig0.substring(66, 130);
  const v0 = '0x' + sig0.substring(130, 132);

  await expect(erc721WrapperVotes.wrapBySig(1, 1, 10e9, v0, r0, s0)).to.not.be
    .reverted;

  await expect(erc721WrapperVotes.unwrap(1)).to.not.be.reverted;

  // replay the same signature and nonce does not revert
  await expect(erc721WrapperVotes.wrapBySig(1, 1, 10e9, v0, r0, s0)).to.not.be
    .reverted;
});

Example Exploit#

It is common for invested third party entities to create user interfaces on top of existing governance contracts. Such parties will often pay for gas on behalf of end users to perform various governance actions. This helps incentivise governance participation / gathering of delegate votes etc...

If such a voting interface is malicious, they could have users sign gasless transactions with arbitrarily high expiry timestamps. This is something non-technical end users may not notice, and even if they are technical the existence of a nonce in the signature implies one time use. Over time, the malicious entity could "hoard" signatures for use in government manipulation. Here is an example exploit.

Total Voting Supply: 100

1. Over time, attacker collects 90 "unwrap" signatures from participating voters
2. Attacker creates a proposal: "Drain the treasury and send it all to me"
3. Attacker acquires 6 delegate votes
4. Shortly before proposal start time, attacker replays all 90 "unwrap" signatures.
5. The tokens are unwrapped and voting supply is reduced to 10
6. The proposal starts with total supply of 10, attacker has 6 / 10 votes, and votes to pass the proposal
7. Treasury is drained

This is obviously a worst case scenario, and the likelihood of such an exploit occurring is a function of how often users wrap and unwrap their tokens, and whether they use a third party interface which sets high expiries.

That being said, there are less catastrophic, but still undesirable results from signature replays being available, such as third parties being able to wrap tokens which are listed on secondary marketplaces thus removing the listing, and other forms of quorum or NFT price floor manipulation.

Recommendation#

Enforce the nonce is unique in both the wrapBySig and unwrapBySig functions:

require(
    nonce == nonces[signatory]++,
    "ERC721Wrapper::wrapBySig: invalid nonce"
);

Additional Recommendation#

If the implementer chooses to further reduce the risk of "signature hoarding" outside of replay attacks, the same nonce mapping can be shared for wrap unwrap and delegate so each new action invalidates any previously saved signatures. This encourages interfaces to submit signatures in a timely manner to avoid invalidation.

They may also wish to implement an invalidatePendingSignatures function which was callable by end users and increments the nonce for a given address in order to invalidate pending signatures.