[M01] Static arguments passed after dynamic arguments

[cylon56]

In the OrderMixin contract, the getTakerAmount and getMakerAmount bytes fields are used as arguments for the _callGetTakerAmount and _callGetMakerAmount functions. These calls provide a way to calculate one side of the swap based on the other side, and they allow users to partially fill orders.

The getTakerAmount/getMakerAmount fields are dynamic variables and are packed in front of the takerAmount and makerAmount values in the _callGetTakerAmount and _callGetMakerAmount functions. It is possible for a malicious maker to provide more data than expected in the getTakerAmount and
getMakerAmount fields to push the takerAmount and makerAmount bytes past where they are assumed to be when being decoded in the next function. This allows the maker to shift the passed in taker or maker amount by a full bytes to the right and even replace them completely if an extra 32 bytes of data is provided.

Users already have to manually review the getTakerAmount and getMakerAmount fields in the order, but this technique is rather hard to spot. Also worth noting, this attack even applies to the internally trusted getMakerAmount and getTakerAmount functions. For most attacks, providing a reasonable threshold amount will prevent loss of funds.

To prevent this, consider encoding the static arguments before the dynamic arguments to avoid giving the dynamic arguments a method to control the static arguments.

Update: Not fixed. The 1inch team stated:

We'll take extra care with getters validation. We'll try to implement sanity validation of getters in our sdk that will help with filtering potentially malicious orders.