addFunds and execute may send tokens twice

[code423n4]

Handle

pauliax

Vulnerability details

Impact

Both calls to IFulfillHelper (addFunds and execute) are wrapped in separate try/catch statements so basically if addFunds succeeds but execute fails or both of these functions fail, the catch will still send assets to the receivingAddress. I think these should be wrapped to a single try/catch to avoid the potential double spend.
Also, I think it is pointless having 2 separate calls (addFunds and execute) as both of them are done one immediately after the other and basically pass the same parameters. One call to the IFulfillHelper could be enough. Eliminating this extra external call will reduce gas costs and simplify the code.

Recommended Mitigation Steps

Make execute payable, send the value, and get rid of addFunds.

[LayneHaber]

#46