search

sherlock-audit / 2024-05-napier-update-judging

8 stars 7 forks source link

bareli - Unchecked ERC20 transfers can cause lock up #87

Closed sherlock-admin3 closed 3 months ago

sherlock-admin3 commented 3 months ago

bareli

medium

Unchecked ERC20 transfers can cause lock up

Summary

Some major tokens went live before ERC20 was finalised, resulting in a discrepancy whether the transfer functions a) should return a boolean or b) revert/fail on error. The current best practice is that they should revert, but return “true” on success. However, not every token claiming ERC20-compatibility is doing this — some only return true/false; some revert, but do not return anything on success. This is a well known issue, heavily discussed since mid-2018.

Today many tools, including OpenZeppelin, offer a wrapper for “safe ERC20 transfer”: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol

Vulnerability Detail

    IERC20(pt.yieldToken()).transfer(recipient, pyAmount);

Impact

transfer may fail.

Code Snippet

https://github.com/sherlock-audit/2024-05-napier-update/blob/main/metapool-router/src/MetapoolRouter.sol#L391

Tool used

Manual Review

Recommendation

use safetransfer instead of transfer.

sherlock-admin2 commented 3 months ago

1 comment(s) were left on this issue during the judging contest.

z3s commented:

Invalid; From README: Twocrypto, Tricrypto LP token, WETH, Napier PT and YT comply with ERC20 standard completely.