The WETH arbitrage profits that are not swapped to SALT will be stuck in `Pools`

[c4-bot-9]

Lines of code

https://github.com/othernet-global/salty-io/blob/6998661013e86a50c7db552d189fadb0521dbeb0/src/pools/Pools.sol#L306-L307

Vulnerability details

Mitigation

commit 6998661 In the pervious implementation, the WETH arbitrage profits will be swapped to SALT immediately in _arbitrage() function. However, it could fail if there is no SALT/WETH liquidity in Pools. The mitigation skipped the swapping step if there is no SALT/WETH liquidity.

Vulnerability details

The WETH arbitrage profits were not swapped to SALT because the mitigation skipped the swapping step under zero SALT/WETH liquidity. However, it doesn't record this for future swapping. As a result, all WETH arbitrage profits obtained under zero SALT/WETH liquidity will be stuck in Pools.

Tools Used

Manual review

Recommended Mitigation Steps

Introduce a variable to accumulate the unswapped arbitrage profits and attempt to swap them for SALT next time.

Assessed type

Other

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[c4-sponsor]

othernet-global (sponsor) acknowledged

[othernet-global]

This is acceptable as it will only happen momentarily when the exchange is launched, and checking another variable for pending WETH to use would increase gas costs forever.

[t0x1cC0de]

Adding comment in PJQA phase:

Hey @Picodes , the sponsor's comment seems to indicate that this is acceptable behaviour and that the suggested fix is not an ideal option. Should this still be a Medium? Isn't entirely clear hence flagging for your final review.

[piken]

The issue indeed exists, and the sponsor has acknowledged it.

[t0x1cC0de]

You are totally right about sponsor's acknowledgement. However, as I came to understand only during the course of this contest, that is not necessarily the deciding factor for accepting a report as non-QA.

The latest example is #126 which the sponsor "confirmed" (not just acknowledged) and also applied the fix, mentioned by him in this comment. But as the judge explained quite clearly and nicely in the discussion forum, there are reasons for it to be marked as QA. Similar examples can be found in round-1 of the contest too like in the case of issue-289 which was sponsor acknowledged. The comments there are instructive to an extent.

This made me realize the final judgement in such cases can only come through the judge's experience, irrespective of sponsor's label. I think one of the factors C4 considers is "Contributing value to sponsors by helping secure their code must always be the central concern", but I am sure there must be numerous other nuances to consider. This might well still continue to be rated as Med after the judge looks at it, but I myself am not entirely sure about the impact of sponsor's last comment and hence thought better to request a final review.

[Picodes]

@t0x1cC0de in this case Medium severity seems justified to me as there is a "leak value with a hypothetical attack path with stated assumptions, but external requirements" although the impact remains small.