Saddle Finance VirtualSwap

Quantstamp7 Mar 2021Files named in the report

Audit

Quantstamp reviewed Saddle (Saddle Finance VirtualSwap) on 7 Mar 2021: 5 findings, 0 critical; all marked fixed.

No open critical findings

Open the report Open Saddle’s security record

Report hosted on certificate.quantstamp.com.

Trustblock does not review code.

Findings5

Statuses are the security firm’s own, from its report.

Severity	Open	Acknowledged	Fixed
Critical	0	0	0
High	0	0	0
Medium	0	0	1
Low	0	0	4

MediumMismatch between inline specification and `withdraw` functionFixed

The comment on L275-276 states: "Reverts if the given `itemId` does not represent a `synthToToken` or a `tokenToSynth` swap." However, the function checks `swapType > PendingSwapType.TokenToSynth`, which allows either `SynthToToken` or `TokenToToken` swap types.

Low`completeToSynth` is callable by any userFixed

The function `completeToSynth` is callable by any user after the `maxSecsLeftInWaitingPeriod`. Although the `synth` wil be sent to the correct `nftOwner` regardless of `msg.sender`, this could have impact on external contracts that rely upon calling `completeToSynth`. For example, suppose some contract has a function of the form: ``` function getSynthAndTransfer(...) { Bridge.completeToSynth(id); synth.transfer(external_addr); } ``` If another arbitrary user invokes `completeToSynth` before the contract invokes it, the funds could be locked forever (assuming the contract did not have sufficient "retrieval" functions).

LowSettle cloned swappers can be destroyedFixed

To save some gas on settlement and storage in general (in case of backup or migration), a cloned `SynthSwapper` that has already been used for settled swaps, can be destroyed. You can also remove the swaps from the pending list.

LowStates `ReadyToSettle`, `PartiallyCompleted` and `Completed` are never checked and can lead to abnormal calls.Fixed

These pending swap states are set but never checked anywhere; this must be due to implicit assumptions on the state of swaps, but opens the door for duplicated calls to, for example, `completeToToken()`. A second duplicated call to `completeToToken()` might be possible with `swapAmount = 0` even if it was completed before.

LowUnchecked function return valuesFixed

In order to ensure that the functions updated state properly, it should be checked that `transfer` calls in `swapSynthToToken` and `withdraw` return true.