C4 token fixes #121
C4 token fixes #121
Conversation
…ch, add tests for token.burnFrom
| @@ -32,7 +31,7 @@ contract OverlayV1OVLCollateral is ERC1155Supply { | |||
| Position.Info[] public positions; | |||
|
|
|||
| IOverlayV1Mothership public immutable mothership; | |||
| IOverlayTokenNew immutable public ovl; | |||
| IOverlayToken immutable public ovl; | |||
There was a problem hiding this comment.
I don't understand the removal of the new token. The auditors found a bug in it that would not have been good. But not one of them chimed in about the pattern itself being a no go.
I don't find a risk in this pattern. It's a highly specialized permissioned function that is only invoked from non manipulable immutable smart contracts.
This is essentially the nature of our entire system. Our entire system does these things and I find no anxiety in collapsing some of the functions in this highly controlled and narrow domain.
| } | ||
|
|
||
| // See: OpenZeppelin Contracts v4.4.0 (token/ERC20/extensions/ERC20Burnable.sol) | ||
| function burnFrom(address _account, uint256 _amount) external onlyBurner { |
There was a problem hiding this comment.
This appears to be contradictory to the advice from one of the reviewers that burn shall not be able to burn from any account.
We should consider if that is a desired system design where tokens are only burnt whence owned by a collateral manager at the time. Burn on transfer in. Burn on transfer out.
A well designed transferBurn function could also satisfy these requirements.
Mitigates issues:
OverlayToken.solCheck of allowance can be done earlier to save gas code-423n4/2021-11-overlay-findings#66