Platform: Code4rena
Start Date: 07/07/2022
Pot Size: $75,000 USDC
Total HM: 32
Participants: 141
Period: 7 days
Judge: HardlyDifficult
Total Solo HM: 4
Id: 144
League: ETH
Rank: 98/141
Findings: 2
Award: $63.34
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0x29A, Amithuddar, Avci, BowTiedWardens, Kthere, Limbooo, MEP, Ruhum, StyxRave, TomJ, Treasure-Seeker, TrungOre, Tutturu, Waze, bardamu, c3phas, cccz, codexploder, cryptphi, hake, horsefacts, hyh, oyc_109, pashov, peritoflores, scaraven, simon135, slywaters, sseefried, tofunmi, xiaoming90
1.3977 USDC - $1.40
https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L172 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L325
call() should be used instead of transfer() on an address payable
This is a classic Code4rena issue:
https://github.com/code-423n4/2021-04-meebits-findings/issues/2 https://github.com/code-423n4/2021-10-tally-findings/issues/20 https://github.com/code-423n4/2022-01-openleverage-findings/issues/75
Impact
The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:
The claimer smart contract does not implement a payable function. The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit. The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call’s gas usage above 2300.
Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.
Impacted lines:
File: 2022-07-fractional\src\modules\Migration.sol 172,29: payable(msg.sender).transfer(ethAmount); 325,29: payable(msg.sender).transfer(userEth);
Recommended Mitigation
I recommend using call() instead of transfer().
#0 - mehtaculous
2022-07-19T21:44:36Z
Duplicate of #325
#1 - HardlyDifficult
2022-07-28T15:45:31Z
Duping to #504
🌟 Selected for report: xiaoming90
Also found by: 0x1f8b, 0x29A, 0x52, 0xA5DF, 0xDjango, 0xNazgul, 0xNineDec, 0xf15ers, 0xsanson, 0xsolstars, 242, 8olidity, Amithuddar, Aymen0909, Bnke0x0, BowTiedWardens, David_, Deivitto, ElKu, Funen, Hawkeye, IllIllI, JC, Kaiziron, Keen_Sheen, Kthere, Kulk0, Kumpa, Lambda, MEP, ReyAdmirado, Rohan16, Ruhum, Sm4rty, TomJ, Tomio, Treasure-Seeker, TrungOre, Tutturu, Viksaa39, Waze, _Adam, __141345__, ak1, apostle0x01, asutorufos, async, ayeslick, aysha, bbrho, benbaessler, berndartmueller, c3phas, cccz, chatch, cloudjunky, codexploder, cryptphi, delfin454000, dipp, durianSausage, dy, exd0tpy, fatherOfBlocks, hake, hansfriese, horsefacts, hubble, joestakey, jonatascm, kebabsec, kenzo, kyteg, mektigboy, neumo, oyc_109, pashov, pedr02b2, peritoflores, rajatbeladiya, rbserver, robee, rokinot, s3cunda, sach1r0, sahar, sashik_eth, scaraven, shenwilly, simon135, sorrynotsorry, sseefried, svskaushik, unforgiven, z3s, zzzitron
61.9379 USDC - $61.94
Details & Impact The transferFrom() method is used instead of safeTransferFrom(), presumably to save gas. I however argue that this isn’t recommended because:
1)OpenZeppelin’s documentation discourages the use of transferFrom(), use safeTransferFrom() whenever possible 2)Given that any NFT can be used for the call option, there are a few NFTs (here’s an example) that have logic in the onERC721Received() function, which is only triggered in the safeTransferFrom() function and not in transferFrom()
Recommended Mitigation Steps Call the safeTransferFrom() method instead of transferFrom()
#0 - mehtaculous
2022-07-21T17:04:09Z
Duplicate of #312
#1 - HardlyDifficult
2022-07-26T19:02:31Z
Agree with the sponsor that this is a non-critical best practice. Transfers may fail but no events are emitted, balance doesn't change, and no other negative consequences were identified.
Lowering risk and making this a QA submission for the warden.