feture: refactored ERC6909

[beebozy]

Summary

Changes Made

Checklist

Before submitting this PR, please ensure:

• Code follows the Solidity feature ban - No inheritance, constructors, modifiers, public/private variables, external library functions, using for directives, or selfdestruct

• Code follows Design Principles - Readable, uses diamond storage, favors composition over inheritance

• Code matches the codebase style - Consistent formatting, documentation, and patterns (e.g. ERC20Facet.sol)

• Code is formatted with forge fmt

• Existing tests pass - Run tests to be sure existing tests pass.

• New tests are optional - If you don't provide tests for new functionality or changes then please create a new issue so this can be assigned to someone.

• All tests pass - Run forge test and ensure everything works

• Documentation updated - If applicable, update relevant documentation

Make sure to follow the contributing guidelines.

Additional Notes

[netlify]

👷 Deploy request for compose-diamonds pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	2c34faf

[maxnorm]

You can remove the redundant subfolder ERC6909. The new approach make it easier to add new features to a standard by only defining a new folder with the feature name under one main ERC6909 folder

[beebozy]

Will make the changes and revert before the day ends

[beebozy]

Hi sir, @maxnorm could you review again?

[maxnorm]

• Update balances after the currentAllowance if (line 93)
• Missing sufficient balance check

uint256 fromBalance = s.balanceOf[_from][_id];
if (fromBalance < _amount) {
    revert ERC6909InsufficientBalance(_from, fromBalance, _amount, _id);
}
unchecked {
    s.balanceOf[_from][_id] = fromBalance - _amount;
}

similar to the transferFrom of the ERC6909TransferFacet

[maxnorm]

@beebozy Please review this too. What do you think of making the balance check and update after the allowance check and update?

[maxnorm]

After reviewing a bit more, i would do something like this:

function burnFrom(address _from, uint256 _id, uint256 _amount) {
    if (_from == address(0)) {
        revert ERC6909InvalidSender(_from);
    }

    ERC6909Storage storage s = getStorage();

    uint256 currentAllowance = s.allowance[_from][msg.sender][_id];
    
    if (msg.sender != _from && currentAllowance < type(uint256).max) {
        if (currentAllowance < _amount) {
            revert ERC6909InsufficientAllowance(msg.sender, currentAllowance, _amount, _id);
        }
        unchecked {
            s.allowance[_from][msg.sender][_id] = currentAllowance - _amount;
        }
    }

    uint256 fromBalance = s.balanceOf[_from][_id];
    if (fromBalance < _amount) {
        revert ERC6909InsufficientBalance(_from, fromBalance, _amount, _id);
    }
    unchecked {
        s.balanceOf[_from][_id] = fromBalance - _amount;
    }

    emit Transfer(msg.sender, _from, address(0), _id, _amount);
}

Current issues in the code:

• Only msg.sender with allowance can burn (no msg.sender is _from or operator)
• No check if _from have a sufficient balance to burn the amount
• we always emit the Transfer event even if the burn isn't done

Question that need reflexion: do we allow an operator to burn tokens? (in the code proposed above, i just allowed the msg.sender to the the _from)

[beebozy]

Fixed @maxnorm kindly review sir..I have a question, declaring the storage before the normal check does it introduce a bug, or it does not follow the normal security pattern.. I was thinking the check and the storage are different things.

[maxnorm]

@beebozy Please review this too. What do you think of making the balance check and update after the allowance check and update?

[maxnorm]

Fixed @maxnorm kindly review sir..I have a question, declaring the storage before the normal check does it introduce a bug, or it does not follow the normal security pattern.. I was thinking the check and the storage are different things.

Hey @beebozy, you are right these a 2 different things. I'm mostly in order to fail fast and avoid unnecessary code execution if the check is true.

Practically, both ways would work as expected without security issues.

[beebozy]

@maxnorm fixed..So sorry for the back and fort.. Kindly review

[maxnorm]

After reviewing a bit more, i would do something like this:

function burnFrom(address _from, uint256 _id, uint256 _amount) {
    if (_from == address(0)) {
        revert ERC6909InvalidSender(_from);
    }

    ERC6909Storage storage s = getStorage();

    uint256 currentAllowance = s.allowance[_from][msg.sender][_id];
    
    if (msg.sender != _from && currentAllowance < type(uint256).max) {
        if (currentAllowance < _amount) {
            revert ERC6909InsufficientAllowance(msg.sender, currentAllowance, _amount, _id);
        }
        unchecked {
            s.allowance[_from][msg.sender][_id] = currentAllowance - _amount;
        }
    }

    uint256 fromBalance = s.balanceOf[_from][_id];
    if (fromBalance < _amount) {
        revert ERC6909InsufficientBalance(_from, fromBalance, _amount, _id);
    }
    unchecked {
        s.balanceOf[_from][_id] = fromBalance - _amount;
    }

    emit Transfer(msg.sender, _from, address(0), _id, _amount);
}

Current issues in the code:

• Only msg.sender with allowance can burn (no msg.sender is _from or operator)
• No check if _from have a sufficient balance to burn the amount
• we always emit the Transfer event even if the burn isn't done

Question that need reflexion: do we allow an operator to burn tokens? (in the code proposed above, i just allowed the msg.sender to the the _from)