Unable to change token approval when tokenAddress changed

[code423n4]

Handle

gzeon

Vulnerability details

Impact

Lock manager and beneficiary can call approveBeneficiary to set the allowance of tokenAddress. However, if tokenAddress is changed by updateKeyPricing, they will not be able to revoke the token approval set previously.

Proof of Concept

https://github.com/unlock-protocol/unlock/blob/025ed6ab14c10cc41d7fe14ab49a051647211adb/smart-contracts/contracts/mixins/MixinLockCore.sol#L226

Recommended Mitigation Steps

  function approveBeneficiary(
    address _spender,
    address _tokenAddress,
    uint _amount
  ) public
    onlyLockManagerOrBeneficiary
    returns (bool)
  {
    return IERC20Upgradeable(_tokenAddress).approve(_spender, _amount);
  }

[julien51]

I am actually not sure they should revoke these. Once the lock has approved a 3rd party address to withdraw some funds from the lock, changing the currency of the lock should not necessarily have an impact on the previous approval.
Example: my lock charges 10 DAI per membership. I allow a designer to withdraw up to 100 DAI. Eventually I switch to USDC, but I don't think I should prevent the designer from withdrawing their 100DAI.

[0xleastwood]

I think the sponsor is right here. Marking as non-critical.