Token Can Deny Execution of `sweepFees()` Function

[code423n4]

Handle

leastwood

Vulnerability details

Impact

The sweepFees() function utilises OpenZeppelin's SafeERC20 library to ensure token transfers are successful. In the event of an unsuccessful token transfer, the transaction will revert with a relevant revert message. However, due to the nature of these reverts, it's possible for one of these tokens to intentionally or unintentionally revert to deny execution of the initial sweepFees() call. This may result in considerable gas costs if the function has already iterated a number of times.

Proof of Concept

https://github.com/code-423n4/2021-10-tally/blob/main/contracts/swap/Swap.sol#L243-L259

Tools Used

Manual code review

Recommended Mitigation Steps

Consider implementing Solidity's try and catch statement to correctly emit an event if a single token sweep was unsuccessful without denying execution of the entire call to sweepFees().

[Shadowfiend]

Since sweepFees takes the token list to sweep as a parameter, the underlying problem can be mitigated with no additional code changes; in particular, the function can be called with fewer or different tokens if a token is found to error. A transaction call before a submission should eliminate most instances where this would be an issue.

That said, I'd still like to see if we can land the try/catch changes.

[0xean]

Going to downgrade the severity on this one as I don't see it an availability risk since the caller could modify the token list