[SVA-05M] Inexistent Erasure of Previous Approvals

[pde-rent]

Recommendation:

We advise the code to properly erase any approval that previously existed, ensuring that no lingering approvals to potentially unauthorized swappers remain.

To note, the input and reward token configuration functions will also need to iterate up to the end of the inputs / rewardTokens array respectively to ensure a shrink of the array will also cause approvals to be erased.

Acknowledged - We considered it unnecessary as the retired tokens should remain on the contract nor be drainable by the swapper, which will itself probably be permissioned. Paired with the unusual nature of inputs and rewardTokens turnover, could we consider to lower the severity on this one?

[omniscia-core]

The rationale behind the medium severity level is the fact that all inputs / reward tokens being simultaneously replaced has a low likelihood. As such, it is very likely that partial replacements will occur and in those cases the approval operations would fail causing the inputs and reward tokens to be irreplaceable unless all entries are simultaneously updated and to non-identical values.

This represents a functional flaw in the system as we believe it is a business requirement to be able to "partially" update reward tokens or inputs.

[pde-rent]

Can you provide details on how the approval would fail if an allowance pre-exists? The finding description is about lingering approvals to potentially unauthorized swappers, and not reverts that would be caused by a partial update - unless not ERC20 compliant (eg. USDT on L1), subsequent approve calls should not break the call flow.

The likelyhood of a broken call-flow is very low, and missing revoked approval would not have much impact given that strategies are liquidated before a change of inputs, therefore the vault balance for these lingering approvals is close to 0.

The issue would arrise if we update the inputs again, meaning an old swapper could now be able to consume his approval. This is very unlikely too. For the above reasons we believe this should be downgraded. Or I might be missing something here

[pde-rent]

Similarly to our SVA-04 comment on external allowance update, a swapper's approvals can be revoked externally before a change of inputs through setSwapperAllowance, the implementation of this off-chain flow would prevent the lingering

[omniscia-core]

Hey @pde-rent, thanks for the update. You are correct in that the original exhibit does not concern a non-zero approval over an existing non-zero approval. As a side-note, this particular "flaw" is EIP-20 compliant. In reality, this adjustment came from the early days of the EIP-20 whereby race conditions were a growing concern in relation to the IERC20::approve implementation.

In any case, the original exhibit's impact chapter denotes that the swapper may change but approval may linger in previously configured swappers. In such an example:

• Strategy is configured with Token A and B as well as Swapper A
• Tokens are replaced to only Token A
• Swapper A is replaced by Swapper B
• Tokens are replaced by Token A and Token B

Although unlikely, the above scenario would mean that Swapper A will have a non-zero allowance for Token B even though allowance should only be configured for Swapper B. We will make sure to reduce the severity of this exhibit by one tier to better align with its probability and estimated impact.

[pde-rent]

Thank you @omniscia-core, fixed in 61a382f420740a0fe2adc7b407c83a33bc642035