search

code-423n4 / 2022-07-swivel-findings

0 stars 1 forks source link

Should add try/catch blocks so the existing state changes won't be undone #101

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L89 https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L91 https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L95 https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L97

Vulnerability details

Impact

Detailed description of the impact of this finding.

The state changes of successful past orders entry should not be undone if there is error thrown in the current order

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

For

1) initiateVaultFillingZcTokenInitiate 2) initiateZcTokenFillingVaultInitiate 3) initiateZcTokenFillingZcTokenExit 4) initiateVaultFillingVaultExit

calls, we should wrap them with try-catch clauses

See image https://drive.google.com/file/d/12j-qaK52Z28GsUWKQ92iHfT_-Yw5UQ8T/view

Tools Used

Manual

Recommended Mitigation Steps

None. (I believe my text written above is quite clear)

JTraversa commented 2 years ago

This would lead to poor UX for our users who believe a transaction completed successfully which did not.

When a user wishes to purchase X amount of nTokens, returning them less than that amount is not acceptable.

bghughes commented 2 years ago

I believe the warden is misunderstanding the execution flow of Solidity. Moreover, I agree with @JTraversa and think that the function should revert and not handle an exception (as the warden suggests) so no state is updated at all on a fail.