The commit above primarily adds a modifier to any operation within the pools contract that deals with arbitrage, primarily it limits the ability to do these operations once per block for the caller. The motivation behind this is as stated in code
"prevent splitting large swaps within a single block into smaller ones as doing so allows for greater price manipulation without consequence from the arbitrage rebalancing."
Please note the commit also introduced a new constant "uint256 constant MAXIMUM_SWAPS_PER_BLOCK = 10;" which was never used, and subsequently removed in further commits, but it seems the intention here was not only to limit the users transaction's to once per block but to also limit the total number of swaps per block, however this strategy was abandoned.
While this limitation of one transaction including arbitrage per block will protect against manipulation, some thought should also go into whether there might be any unintentional side effects to this, for instance this might impact trading strategies of well intentioned users. Also to note that the mitigation does not entirely remove the risk of manipulation. An Attacker can always split his transaction across multiple accounts, albeit this might seem unreasonable to pull in practice due to gas costs.
Lines of code
Vulnerability details
Lines of code
Vulnerability details
Lines of code
Vulnerability details
Additional Scope Issue
https://github.com/othernet-global/salty-io/commit/2d1b7df004394720c0d8bb4aefe903021631eff3
Comments
The commit above primarily adds a modifier to any operation within the pools contract that deals with arbitrage, primarily it limits the ability to do these operations once per block for the caller. The motivation behind this is as stated in code
"prevent splitting large swaps within a single block into smaller ones as doing so allows for greater price manipulation without consequence from the arbitrage rebalancing."
Please note the commit also introduced a new constant "uint256 constant MAXIMUM_SWAPS_PER_BLOCK = 10;" which was never used, and subsequently removed in further commits, but it seems the intention here was not only to limit the users transaction's to once per block but to also limit the total number of swaps per block, however this strategy was abandoned.
While this limitation of one transaction including arbitrage per block will protect against manipulation, some thought should also go into whether there might be any unintentional side effects to this, for instance this might impact trading strategies of well intentioned users. Also to note that the mitigation does not entirely remove the risk of manipulation. An Attacker can always split his transaction across multiple accounts, albeit this might seem unreasonable to pull in practice due to gas costs.
Conclusion
LGTM