Add ERC20Freezable, ERC20Restricted and ERC20uRWA

[ernestognw]

ERC-7943 made it as a draft, so I think it's fine to start by implementing the ERC20 version. A couple of notes:

• We already have an ERC20Allowlist and ERC20Blocklist that is offered in Wizard. I feel a better abstraction would be ERC20Restricted, otherwise the ERC20uRWA implementation would keep isUserAllowed virtual to return allowed or blocked accordingly
• I'm not restricting the approve function but we're doing it in ERC20Allowlist and ERC20Blocklist so we need to make sure there's not an alternative path to bypass the restrictions

[xaler5]

What about ERC7943InsufficientUnfrozenBalance ? Is not mandatory but just curious of your thoughts

[ernestognw]

I felt it was not worth introducing concepts outside of ERC20Freezable's domain. So I preferred to keep ERC20InsufficientUnfrozenBalance for consistency with the contract naming. None is objectively better imo, though.

btw I just recalled that we already have an ERC20Custodian that might replace this ERC20Freezable, I just need to take a deeper look

[james-toussaint]

Choosing a default looks opinionated, how about leaving that abstract?

Maybe I misunderstood you initial comment here:

otherwise the ERC20uRWA implementation would keep isUserAllowed virtual to return allowed or blocked accordingly

[ernestognw]

Ah, that comment is related to how the ERC20uRWA was set up in the first place. See this comment.

I agree that a default is opinionated, but I think we don want to be opinionated here to avoid users to make a decision at first. I prefer a non-restrictive default rather than a virtual function

[james-toussaint]

How about adding the two isAllowed(address user) & isNotBlocked(address user) so user can straightfowardly override:

function isUserAllowed(address user) public view override returns (bool) {
  return isNotBlocked(user); // or !isBlocked(..)
}

function isUserAllowed(address user) public view override returns (bool) {
  return isAllowed(user);
}

(?)

[ernestognw]

Yeah, good idea. Though, these functions come in ERC20Allowlist and ERC20Blocklist (i.e. allowed and blocked respectively). Initially ERC20Restricted was not a thing and this was the default recommendation (see here):

// Using ERC20Allowlist
contract MyuRWA20 is uRWA20, ERC20Allowlist {
    function isUserAllowed(address user) public view virtual override returns (bool) {
        return allowed(user);
    }
}

// Using ERC20Blocklist
contract MyuRWA20 is uRWA20, ERC20Blocklist {
    function isUserAllowed(address user) public view virtual override returns (bool) {
        return !blocked(user);
    }
}

I currently prefer the ERC20Freezable with a default "blocklist" behavior, since I believe is the least restrictive option (i.e. an allowlist is just a stricter blocklist)

[james-toussaint]

Suggested change

	abstract contract uRWA20Mock is uRWA20, AccessControl {
	contract uRWA20Mock is uRWA20, AccessControl {

?
Do we need the mock to be abstract?

[ernestognw]

It's not needed but it helps because hardhat-exposed exposes the constructors automatically 😄

EDIT: See cf08cbf

[arr00]

wRWA will always break linting

[arr00]

Can the file name match the contract?

[ernestognw]

I'm almost sure there was a reason why we were restricting approvals in ERC20Allowlist and ERC20Blocklist but can't remember it

[ernestognw]

For clarity, it is not possible to bypass the restriction since every transferFrom would go through _update

[ernestognw]

I think I'll remove this check. Essentially, it duplicates the check that super._update already does given it inherits from ERC20Freezable.

In terms of compliance, the spec states:

Public transfers (transfer, transferFrom, safeTransferFrom, etc.) MUST NOT succeed in cases in which isTransferAllowed or isUserAllowed would return false for either one or both from and to addresses.

So this is still true even if we don't call isTransferAllowed here.

The isTransferAllowed MUST validate that the amount being transferred doesn't exceed the unfrozen amount (which is the difference between the current balance and the frozen balance). Additionally it MUST perform an isUserAllowed check on the from and to parameters.

1. ERC20Freezable._update checks:

   • if (from != address(0)) → validates amount <= available(from) ✅

2. ERC20Restricted._update checks:

   • if (from != address(0)) _checkRestricted(from) → calls isUserAllowed(from) ✅
   • if (to != address(0)) _checkRestricted(to) → calls isUserAllowed(to) ✅

This change would stop emitting the ERC7943NotAllowedTransfer error, but I think that's fine since it's not very expressive and the super._update call will revert with more appropriate errors. The ERC allows to skip reverting with ERC7943NotAllowedTransfer.

As a side effect, I think any override to isTransferAllowed would not be enforced in _update automatically, so I guess we can note it.