Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom

[code423n4]

Handle

cccz

Vulnerability details

Impact

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

Proof of Concept

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L457

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L463

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L489

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L513

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L537

Tools Used

Manual analysis

Recommended Mitigation Steps

Consider using safeTransfer/safeTransferFrom or require() consistently.

[cryptofish7]

Fix: traderjoe-xyz/rocket-joe@9e11786

[dmvt]

This could result in a loss of funds given the right external conditions.

2 — Med (M): vulns have a risk of 2 and are considered “Medium” severity when assets are not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

[amarpatel]

To test, I did this:

on package.json add test:TEMP to scripts:

...
"test:TEMP": "NODE_ENV=test ETHERNAL_ENABLED=false hardhat test --grep \"should process subsequent withdrawals of all supported tokens successfully\" --parallel",
...

Every vault fails: