Platform: Code4rena
Start Date: 14/06/2022
Pot Size: $50,000 USDC
Total HM: 19
Participants: 99
Period: 5 days
Judge: HardlyDifficult
Total Solo HM: 4
Id: 136
League: ETH
Rank: 41/99
Findings: 3
Award: $97.22
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: hyh
Also found by: 0x29A, 0xNineDec, 0xf15ers, 0xkowloon, GreyArt, IllIllI, KIntern, Kenshin, Lambda, WatchPug, Wayne, berndartmueller, byterocket, cccz, codexploder, horsefacts, kenzo, obront, obtarian, oyc_109, peritoflores, rajatbeladiya, rfa, saian, unforgiven, zer0dot
11.084 USDC - $11.08
https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1229-L1232 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L345-L348
The rescueETH() function cannot be used to rescue any ETH transfered/stuck on the contract as the comment written.
@dev used for rescuing exchange fees paid to the contract in tokens The function is just passing
msg.valuefrom caller toaddress destination, not any ETH in the contract.
rescueETH(address destination) with 10 ETH attatched as the value to SEND 10 ETH back to the destination.This is a test script, include it to test/staker.js and run with npx hardhat test test/staker.js
describe('PoC rescueETH', () => { it.only('Cannot transfer ETH out of the contract', async function () { // send ETH to the contract let oneETH = ethers.utils.parseEther('1'); expect(await ethers.provider.getBalance(infinityStaker.address)).to.equal(0); let signer2BalanceBefore = await ethers.provider.getBalance(signer2.address) let tx = await signer2.sendTransaction({ to: infinityStaker.address, value: oneETH }) let signer2BalanceAfter = await ethers.provider.getBalance(signer2.address) expect(await ethers.provider.getBalance(infinityStaker.address)).to.equal(oneETH) expect(signer2BalanceBefore).to.be.at.least(signer2BalanceAfter.add(oneETH)) // before >= after, left some room for gas // admin rescue that ETH let adminBalanceBefore = await ethers.provider.getBalance(signer1.address) await infinityStaker.connect(signer1).rescueETH(signer2.address, {value: oneETH}) let adminBalanceAfter = await ethers.provider.getBalance(signer1.address) expect(await ethers.provider.getBalance(signer2.address)).to.equal(signer2BalanceAfter.add(oneETH)) // admin balance decreased while contract balance still the same. expect(adminBalanceBefore).to.be.at.least(adminBalanceAfter.add(oneETH)) // before >= after, left some room for gas expect(await ethers.provider.getBalance(infinityStaker.address)).to.equal(oneETH) }) })
In order to make this function work as its name, the function must receive uint amount as another input to transfer that amount out of the contract to destination address. And the payable can be removed to prevent any callings with ETH attached.
#0 - nneverlander
2022-06-22T19:10:00Z
Duplicate
#1 - nneverlander
2022-07-05T12:37:57Z
#2 - HardlyDifficult
2022-07-09T16:57:18Z
🌟 Selected for report: joestakey
Also found by: 0x1f8b, 0x29A, 0x52, 0xDjango, 0xNazgul, 0xNineDec, 0xf15ers, 0xkowloon, 0xmint, 8olidity, BowTiedWardens, Chom, Cityscape, Czar102, ElKu, FSchmoede, Funen, GimelSec, GreyArt, IllIllI, KIntern, Kaiziron, Kenshin, Lambda, MadWookie, MiloTruck, PPrieditis, Picodes, Ruhum, Sm4rty, StErMi, TerrierLover, TomJ, Treasure-Seeker, VAD37, WatchPug, Wayne, _Adam, a12jmx, abhinavmir, antonttc, apostle0x01, asutorufos, berndartmueller, cccz, cloudjunky, codexploder, cryptphi, csanuragjain, defsec, delfin454000, fatherOfBlocks, georgypetrov, hake, hansfriese, horsefacts, hyh, k, kenta, nxrblsrpr, oyc_109, peritoflores, rajatbeladiya, reassor, rfa, robee, sach1r0, saian, samruna, shenwilly, simon135, sorrynotsorry, sseefried, throttle, unforgiven, wagmi, zzzitron
54.7062 USDC - $54.71
Important or state changes function should emit events upon successful execution for off-chain tracking.
An event of calling critical functions should be generated for security and off-chain monitoring purposes.
NFT transfering function can be called with address zero (0x00...00) as the destination. This might cause unexpected behavior or unintentionally burn.
It is recommended to validate that the destination address should not be address zero to prevent from unintentionally burn.
PROTOCOL_FEE_BPS Can Be Set To 100% or AboveThere is no maximum limit on how maximum the PROTOCOL_FEE_BPS can be, which might result in a fee rate at 100%, meaning the protocol will collect the entire trading amount. Additionally, it can be more than 10000 (100%) and will result in Denial of Service due to overflow reverting.
It is recommended to determine how high the fee can be at maximum and add the validation to ensure that the fee cannot be set higher than the maximum value.
The reported require statement is validating on tautology logic, the condition that will always be true. From the fact that the default value of uint in Solidity is 0, so, the condition like >= 0 will always be true in any circumstances.
The condition can be changed from >= 0 to > 0.
The rage-quit penalty can be set to zero which will causing rage quit always be reverted due to divide by zero.
A zero-value validation check should be included in the penalty setter function.
🌟 Selected for report: IllIllI
Also found by: 0v3rf10w, 0x1f8b, 0x29A, 0xAsm0d3us, 0xDjango, 0xKitsune, 0xNazgul, 0xf15ers, 0xkatana, 0xkowloon, BowTiedWardens, Chom, ElKu, FSchmoede, Funen, GimelSec, Kaiziron, Kenshin, Lambda, MadWookie, MiloTruck, PPrieditis, Picodes, PwnedNoMore, StErMi, Tadashi, TerrierLover, TomJ, Tomio, Wayne, Waze, _Adam, antonttc, apostle0x01, asutorufos, c3phas, codexploder, defsec, delfin454000, fatherOfBlocks, hake, hansfriese, hyh, joestakey, k, kenta, oyc_109, peritoflores, reassor, rfa, robee, sach1r0, simon135, slywaters, zer0dot
31.4283 USDC - $31.43
Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them. Source: https://blog.soliditylang.org/2021/04/21/custom-errors/
InfinityExchange.solInfinityOrderBookComplication.solInfinityStaker.solInfinityToken'solConsider using custom errors instead if the contract uses Solidity version 0.8.4 or above.
EVM is a stack machine with 256 bits (32 bytes) for each stack. Using unnecessary information that has size more than 32 bytes requires more than one stacks for storing, therefore using more gas unnecessarily.
InfinityExchange.solInfinityStaker.solUse a string that not bigger than 32 bytes or consider using custom errors instead if the contract uses solidity version 0.8.4 or above.
unchecked Block Can Be Used To Save More GasFrom Solidity version 0.8.0 and above, the arithmetic overflow/underflow has been included in the compiler-level. Any arithmetic overflow/underflow will always be reverted by default, which mean it costs more gas by default for overflow/underflow validation. In case that any arithmetic operations can be gaurantee to not be overflown/underflown the unchecked block can be used to save more gas.
The line 99 can be gaurantee to not be overflown/underflown by the require statement at line 93. Therefore, consider apply unchecked with the line to save more gas.
getVestedAmount() Logic Can Save More Gas For On-Chain CallingThe getVestedAmount() will return right after entering if the stored retrieved timestamp is 0, in that case, the declared uint256 amount will never be used.
Reorder the line 261 that declaring and reading storage to be after the if-statement as demonstrated below.
function getVestedAmount(address user, Duration duration) public view returns (uint256) { uint256 timestamp = userstakedAmounts[user][duration].timestamp; // short circuit if no vesting for this duration if (timestamp == 0) { return 0; } uint256 amount = userstakedAmounts[user][duration].amount; // Move to after the if-statement uint256 durationInSeconds = _getDurationInSeconds(duration); uint256 secondsSinceStake = block.timestamp - timestamp; return secondsSinceStake >= durationInSeconds ? amount : 0; }