Swivel.setFee() is implemented wrongly.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Swivel/Swivel.sol#L507

Vulnerability details

Impact

Swivel.setFee() is implemented wrongly. Swivel.feenominators won't be set as expected.

Proof of Concept

This function has a parameter "i" for the index of the new fee denomination but it isn't used during the update.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

This line should be modified like below.

feenominators[i[x]] = d[x];

[JTraversa]

Given this allows us to change fees post initialization, and doesnt lead to the leakage of value or loss of funds, but a potential issue for admins solely (in a rare edge case where fees would even need to be changed), I might consider this low risk?

[bghughes]

Given this allows us to change fees post initialization, and doesnt lead to the leakage of value or loss of funds, but a potential issue for admins solely (in a rare edge case where fees would even need to be changed), I might consider this low risk?

I agree with the warden here given that this is an incorrect implementation of the intended functionality. The warden's suggestion shows that the function was not working as intended; if in production, for instance, this was not caught then calls to setFee would not work as intended and not set any fees across the markets. Instead, it would only populate the zero index of the 2nd-demensional array uint16[4] public feenominators; losing these values [zcTokenInitiate, zcTokenExit, vaultInitiate, vaultExit] and breaking functionality.

[JTraversa]

Yeah I was kind of on the fence on this one thinking the method wouldnt impact the current feenominators in the way youve stated, but because it actually could have an impact, and the event itself also would be misleading, removing the disagreement.

[robrobbins]

addressed: https://github.com/Swivel-Finance/gost/pull/419