Uniswap / v3-periphery

🦄 🦄 🦄 Peripheral smart contracts for interacting with Uniswap v3
https://uniswap.org
GNU General Public License v2.0
1.15k stars 1.06k forks source link

remove redundant payable keyword #354

Closed tanliwei closed 5 months ago

tanliwei commented 11 months ago

The payable keywords for functions in the SelfPermit contract are redundant.

Per the specification of the EIP-2612, the permit function does not need a payable keyword.

function permit(address owner, address spender, uint value, uint deadline, uint8 v, bytes32 r, bytes32 s) external

The test file for the SelfPermit contract does not verify the payable related logic, which implies the payable keyword is unnecessary:

...
  describe('#selfPermit', () => {
    const value = 456

    it('works', async () => {
      const { v, r, s } = await getPermitSignature(wallet, token, selfPermitTest.address, value)

      expect(await token.allowance(wallet.address, selfPermitTest.address)).to.be.eq(0)
      await selfPermitTest.selfPermit(token.address, value, constants.MaxUint256, v, r, s)
      expect(await token.allowance(wallet.address, selfPermitTest.address)).to.be.eq(value)
    })

    it('fails if permit is submitted externally', async () => {
      const { v, r, s } = await getPermitSignature(wallet, token, selfPermitTest.address, value)

      expect(await token.allowance(wallet.address, selfPermitTest.address)).to.be.eq(0)
      await token['permit(address,address,uint256,uint256,uint8,bytes32,bytes32)'](
        wallet.address,
        selfPermitTest.address,
        value,
        constants.MaxUint256,
        v,
        r,
        s
      )
      expect(await token.allowance(wallet.address, selfPermitTest.address)).to.be.eq(value)

      await expect(selfPermitTest.selfPermit(token.address, value, constants.MaxUint256, v, r, s)).to.be.revertedWith(
        'ERC20Permit: invalid signature'
      )
    })
  })
...

On the other hand, the payable keyword increases the risk of locking ether, due to the lack of the ether withdraw function in the SelfPermit contract and its derivative contracts.

Recommend removing the redundant payable keywords and reducing the risk of locking the ether.

stale[bot] commented 5 months ago

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.