morpho-org/morpho-blue

Slightly wrong external requirement

Closed this issue ยท 2 comments

https://github.com/morpho-labs/morpho-blue/blob/7d60ca06e827780115d90508339de5560be9f643/src/interfaces/IMorpho.sol#L134-L135

The token balance of Morpho should only decrease on transfer and transferFrom. In particular, tokens with burn functions are not supported.

Actually as long as the function is gated to the owner of the tokens it's ok. In particular burn functions which needs to be called by the owner of the token are actually supported. Because it might be quite comment, I would adapt the comment.

I agree.

I would like to open the discussion for this sentence as well:
https://github.com/morpho-labs/morpho-blue/blob/7d60ca06e827780115d90508339de5560be9f643/src/interfaces/IMorpho.sol#L136
I don't see any issue with tokens having reentrancy capacities (such as ERC20 tokens implementing ERC777)

Those requirements are not wrong: they are requirements for which we know that the market behaves as expected, they are not equivalent conditions.

Couple of remarks about this:

  • the gain when doing the change is to annouce that more tokens could be onboarded. Are we sure that this is required, is ERC777 important for example ?
  • those requirements are not testable, because we cannot enumerate all the behaviors that a token can have. Because of this, I would advise against doing such change unless we are extremely confident about it.
  • it can be formally verified, but it's not easy. It will probably require to scope what the token can do. I can check what breaks with looser invariants though