[Audit] Revised Payment Order Struct

[marvinkruse]

This PR contains changes related to our revised Payment Order Struct. A payment order is a formalized set of information used by payment clients and are sent to a payment processor, who acts based upon the information contained in the order.

What have we done
We revamped the struct and made a distinction between base information (i.e. amount, token, chainId, etc.) and additional information, where we saw that the additional information needs to allow for certain changes over time. We ultimately arrived at this struct:

struct PaymentOrder {
    address recipient;    
    address paymentToken;
    uint amount;
    uint originChainId;
    uint targetChainId;        
    bytes32 flags;
    bytes32[] data;
}

The way the flags and data fields work is as follows:
The flags field encodes which information is contained in the data array. The way this happens is similar to a bit-mask, where each of the bits in the bytes32 variable represents one potential data type. We currently have these slots defined:

    | Flag | Variable type | Name       | Description                         |
    |------|---------------|------------|-------------------------------------|
    | 0    | bytes32       | orderID    | ID of the order.                    |
    | 1    | uint256       | start      | Start date of the streaming period. | 
    | 2    | uint256       | cliff      | Duration of the cliff period.       |
    | 3    | uint256       | end        | Due Date of the order               |
    | ...  | ...           | ...        | (yet unassigned)                    |
    | 255  | .             | .          | (Max Value).                        | 
    |------|---------------|------------|-------------------------------------|

So in this example, if we want to make use of the streamed payment processor and want to encode the start, cliff, and end times, we would set the flags value to be 0000 [...] 0000 1110, so the flags at position 1, 2 and 3 and set to 1, while the rest is set to 0. If the used payment processor allows for start, cliff, and end values, it will check whether these slots are set (i.e. are 1) and - if that is the case - extract these values from the array.

The data array only contains values for slots where the flag is 1, so for the example above (0000 [...] 0000 1110) it would contain three data entries (length of 3), where the first one is from flag 1 and so on.

The documentation around this and the definition of the available slots can be found in IERC20PaymentClientBase_v1.sol.

[omega-audits]

If default start and end are 0 you may want to have them as block.timestamp, it seems like this option could be useful as you would usually want the default to be whatever block.timestamp is, not a specific date.

[0xNuggan]

Thanks fo the tip! While it makes sense, we've chosen to go against it since being able to default variables to 0 (and therefore create valid payment Orders, even if they are immediately claimable), seems to me better than introducing block.timestamp values that the user might not expect.
I've created a linear issue to track this, in case we choose to look for a different way to handle it.

[Zitzak]

It's missing the comment update merged into dev, which reverted this comment. We prob need to rebase/merge from dev.

Just something I noticed while working on another branch

[marvinkruse]

Yes, good reminder: please rebase from dev so this is up to date. I can also do this if you want (@0xNuggan) - lmk!

[marvinkruse]

Thank you for adressing my overall comment of the data array ordering thing I mentioned, to me it looks like that was fully addressed. We probably just need to make sure that each payment processor does it like you did it here (i.e. applies the same principle). May be worth to look into creating a library or a base here for payment processors that handles this?

[marvinkruse]

Tagging @0xNuggan - as this is my PR I can't explicitly tag my comments as requests for changes, so just letting you know via this comment that I submitted my review.

Feel free to close any comments that you feel like you properly addressed and just leave anything open you feel like I should look at again/you have comments on/etc.

[0xNuggan]

Thanks for the review! Addressed the comments

[0xNuggan]

About moving the array logic into a library I agree, but I can't think of a good place right now where we shoudl put it. But worth keeping in mind.

[marvinkruse]

Looks good, just small typos.

[jellegerbrandy]

Would it not be better (safer) if this is an error? This after all is not a case where the order does not provide a value, it is a case where the "order type" is different from what is expected (I mean that if the flag value is different, this is not the same as providing a "0" value and have the PP fill that in with a default value)
For example, if the order says "This is an order with no start and end date", then it is it is better that a PP that is made for streaming orders says "Ok, you probably made a istake sending me this order" rather than have it say "O, ok, then I will just make it a streaming order which starts in 17 days and lasts 189 hours"

[0xNuggan]

It's intended behavior, since we want to give the PaymentProcessor to build a valid PaymentOrder using its defaults. I've added a check that the resulting PaymentOrder after "reconstruction" is valid.

[0xNuggan]

Closing this in favor or #717