Pool.removeForMember(address), Pool.swapTo(address,address) and Pool.burnSynth(address,address) on lines 198, 199, 224, 250, and 253 ignore the boolean return on transfers. This is a brittle implementation because it relies on the boolean return value being hard-coded to true. Potentially a token may return a false instead of reverting and this opens the possibility of unexpected outcomes for removals, swaps and burns.
Note also that Pool.swapTo(address,address) emits a potentially erroneous Swapped() event on line 223 due to the above unchecked transfer.
SamusElderg
commented
3 years ago
Handle
heiho1
Vulnerability details
Impact
Pool.removeForMember(address), Pool.swapTo(address,address) and Pool.burnSynth(address,address) on lines 198, 199, 224, 250, and 253 ignore the boolean return on transfers. This is a brittle implementation because it relies on the boolean return value being hard-coded to true. Potentially a token may return a false instead of reverting and this opens the possibility of unexpected outcomes for removals, swaps and burns.
Note also that Pool.swapTo(address,address) emits a potentially erroneous Swapped() event on line 223 due to the above unchecked transfer.
Proof of Concept
https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Pool.sol#L198
https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Pool.sol#L199
https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Pool.sol#L224
https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Pool.sol#L250
https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Pool.sol#L253
Tools Used
Slither
Recommended Mitigation Steps
There is no particular disadvantage to a
require(success, "!transfer")check.Events should not be emitted prior to confirmation of successful transfer or the logs may become polluted.