Platform: Code4rena
Start Date: 22/05/2024
Pot Size: $20,000 USDC
Total HM: 6
Participants: 126
Period: 5 days
Judge: 0xsomeone
Total Solo HM: 1
Id: 379
League: ETH
Rank: 49/126
Findings: 2
Award: $0.02
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: SpicyMeatball
Also found by: 0rpse, 0xMosh, 0xblack_bird, 0xdice91, 0xhacksmithh, 0xleadwizard, 0xmystery, Audinarey, AvantGard, Bigsam, Dots, EPSec, Eeyore, Janio, Limbooo, LinKenji, Mahmud, MrPotatoMagic, Myd, Oxsadeeq, Sabit, SovaSlava, Stefanov, Tychai0s, Utsav, Varun_05, Walter, adam-idarrha, ahmedaghadi, araj, aslanbek, ayden, bigtone, c0pp3rscr3w3r, carrotsmuggler, crypticdefense, dhank, fyamf, gajiknownnothing, gavfu, itsabinashb, jasonxiale, joaovwfreire, ke1caM, leegh, merlinboii, mitko1111, n4nika, pfapostol, prapandey031, rouhsamad, sandy, snakeeaterr, stakog, steadyman, swizz, tedox, th3l1ghtd3m0n, trachev, turvy_fuzz, xyz, yashgoel72, zhaojohnson
0.0105 USDC - $0.01
In the setLockDuration function, the calculation of unlockTime using lastLockTime + uint32(_duration) can set the unlockTime to a value earlier than the current unlockTime if lastLockTime is in the past. This can unintentionally reduce the unlock time, violating the intended logic.
In the setLockDuration function, the unlockTime is calculated using lastLockTime + uint32(_duration), which can result in an earlier unlock time if lastLockTime is in the past:
function setLockDuration(uint256 _duration) external notPaused { if (_duration > configStorage.getUint(StorageKey.MaxLockDuration)) revert MaximumLockDurationError(); playerSettings[msg.sender].lockDuration = uint32(_duration); // update any existing lock uint256 configuredTokensLength = configuredTokenContracts.length; for (uint256 i; i < configuredTokensLength; i++) { address tokenContract = configuredTokenContracts[i]; if (lockedTokens[msg.sender][tokenContract].quantity > 0) { // check they are not setting lock time before current unlocktime if ( uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime ) { revert LockDurationReducedError(); } uint32 lastLockTime = lockedTokens[msg.sender][tokenContract] .lastLockTime; lockedTokens[msg.sender][tokenContract].unlockTime = lastLockTime + uint32(_duration); } } emit LockDuration(msg.sender, _duration); }
Manual Review
Use block.timestamp directly to ensure that the new unlockTime is always in the future relative to the current block time. This avoids issues with lastLockTime being in the past:
function setLockDuration(uint256 _duration) external notPaused { if (_duration > configStorage.getUint(StorageKey.MaxLockDuration)) revert MaximumLockDurationError(); playerSettings[msg.sender].lockDuration = uint32(_duration); // update any existing lock uint256 configuredTokensLength = configuredTokenContracts.length; for (uint256 i; i < configuredTokensLength; i++) { address tokenContract = configuredTokenContracts[i]; if (lockedTokens[msg.sender][tokenContract].quantity > 0) { // check they are not setting lock time before current unlocktime if ( uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime ) { revert LockDurationReducedError(); } uint32 lastLockTime = lockedTokens[msg.sender][tokenContract] .lastLockTime; lockedTokens[msg.sender][tokenContract].unlockTime = uint32(block.timestamp) + uint32(_duration); } } emit LockDuration(msg.sender, _duration); }
This ensures that the unlockTime is always set to a future time relative to the current block time, preventing any unintended reduction in the unlock time.
Context
#0 - c4-judge
2024-06-04T12:41:06Z
alex-ppg marked the issue as duplicate of #89
#1 - c4-judge
2024-06-05T12:53:18Z
alex-ppg marked the issue as partial-75
🌟 Selected for report: robertodf99
Also found by: 0xAadi, 0xAkira, 0xdice91, 0xhacksmithh, 0xleadwizard, AgileJune, Bauchibred, Bbash, Beosin, Bigsam, Dots, EPSec, EaglesSecurity, Eeyore, Evo, John_Femi, Mahmud, MrPotatoMagic, RotiTelur, Rushkov_Boyan, Sabit, Sentryx, Stormreckson, Topmark, Tychai0s, Utsav, Walter, ZanyBonzy, ZdravkoHr, adam-idarrha, araj, aslanbek, avoloder, bigtone, brevis, brgltd, carrotsmuggler, crypticdefense, dd0x7e8, dhank, djanerch, falconhoof, iamandreiski, joaovwfreire, leegh, merlinboii, mitko1111, pamprikrumplikas, pfapostol, prapandey031, swizz, trachev, twcctop, typicalHuman, unique, xyz
0.0148 USDC - $0.01
A price feed can approve a USD price proposal after previously disapproving it. This inconsistency can lead to manipulation and unreliable approval processes, undermining the integrity of the price update mechanism.
In the approveUSDPrice function, there is no check to see if the caller has previously disapproved the same price proposal. This allows a price feed to both disapprove and then approve the same proposal:
function approveUSDPrice(uint256 _price) external onlyOneOfRoles([...]) { if (usdUpdateProposal.proposer == address(0)) revert NoProposalError(); if (usdUpdateProposal.proposer == msg.sender) revert ProposerCannotApproveError(); if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId) revert ProposalAlreadyApprovedError(); if (usdUpdateProposal.proposedPrice != _price) revert ProposalPriceNotMatchedError(); // Missing check for previous disapproval usdUpdateProposal.approvals[msg.sender] = _usdProposalId; usdUpdateProposal.approvalsCount++; if (usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD) { _execUSDPriceUpdate(); } emit ApprovedUSDPrice(msg.sender); }
Manual Review
Add a check in the approveUSDPrice function to ensure that the caller has not previously disapproved the same price proposal:
function approveUSDPrice(uint256 _price) external onlyOneOfRoles([...]) { if (usdUpdateProposal.proposer == address(0)) revert NoProposalError(); if (usdUpdateProposal.proposer == msg.sender) revert ProposerCannotApproveError(); if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId) revert ProposalAlreadyApprovedError(); if (usdUpdateProposal.proposedPrice != _price) revert ProposalPriceNotMatchedError(); if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId) revert ProposalAlreadyDisapprovedError(); // Add this line usdUpdateProposal.approvals[msg.sender] = _usdProposalId; usdUpdateProposal.approvalsCount++; if (usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD) { _execUSDPriceUpdate(); } emit ApprovedUSDPrice(msg.sender); }
This ensures that a price feed cannot approve a proposal after previously disapproving it, maintaining the integrity of the approval process.
Invalid Validation
#0 - c4-judge
2024-06-05T12:42:22Z
alex-ppg marked the issue as satisfactory