code-423n4 / 2023-08-pooltogether-findings

4 stars 3 forks source link

UNCHECKED TRANSFER #158

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngAuction.sol#L182

Vulnerability details

Impact

Some tokens do not revert the transaction when transferFrom fails and returns False. Hence we must check the return value after calling the transfer or transferFrom function.

Proof of Concept

Check the last answer here: https://ethereum.stackexchange.com/questions/136039/what-is-best-practice-for-transferfrom-out-of-these-two-ways In short: Using token.transferFrom(msg.sender, addr, amount); is NOT enough. The risk is that you're assuming the transfer was successful when actually it wasn't. Example of tokens that don't revert: ZRX, HT, WOO. The safest method is using this:

(bool success, bytes memory data) = address(token).call(abi.encodeWithSelector(token.transferFrom.selector, from, to, value)); require(success && (data.length == 0 || abi.decode(data, (bool))), 'token transfer from sender failed'); This is the one used by Openzeppelin's SafeERC20 library.

Tools Used

Manual Report

Recommended Mitigation Steps

Use OpenZeppelin SafeERC20's safetransfer and safetransferFrom functions.

Assessed type

Token-Transfer

c4-pre-sort commented 1 year ago

raymondfam marked the issue as low quality report

raymondfam commented 1 year ago

A known issue in the bot.

c4-judge commented 1 year ago

HickupHH3 marked the issue as unsatisfactory: Invalid