Contract fails to deliver promised returns, but doesn't lose value
Description
Brief/Intro
Summary:
Due to seemingly accidental oversight the staleThreshold variable was accidentally left at a value suitable for testing phase only. The original value was 30 days, but the current value is 60 days.
30 days and older would normally be deemed stale prices, but since 60 days is currently used it means only 60 days and older would now be deemed stale prices, so if prices between 30 days and 60 days are used, they will be accepted as valid prices when they are actually stale prices, and the protocol/users wont be aware of the problem.
Vulnerability Details
Impact Details
Impact:
Most likely impact in scope: Contract fails to deliver promised returns, but doesn't lose value
Likelihood: medium Impact: high Severity: low - medium
Potential impacts:
compounding lower/higher amount than expected versus if the price was not stale, so the user/voter would get either more or less rewards than they should have received
potential protocol instability due to unstable/invalid pricing
and test run command: make test_file_debug_test FILE=Minter TEST=testCompoundRewards
Function under test:
I made some modifications for PoC testing purposes, to simplify the testing process, see my audit tag comments below:
functionamountToCompound(uint256_alcxAmount) publicviewreturns (uint256,uint256[] memory) {// Increased for testing since tests go into future uint256 staleThreshold = 60 days; /// @audit-issue probably incorrect due to above comment, and previous value was: `uint256 staleThreshold = 30 days;`
//uint256 staleThreshold = 30 days; /// @audit added for PoC/testing purposes (uint80 roundId,int256 alcxEthPrice,,uint256 priceTimestamp,uint80 answeredInRound) = priceFeed .latestRoundData(); priceTimestamp = block.timestamp - 59 days; /// @audit added for PoC/testing purposes >>> overriding the variable from above for the specific PoC test
require(answeredInRound >= roundId,"Stale price");require(block.timestamp - priceTimestamp < staleThreshold,"Price is stale");require(alcxEthPrice >0,"Chainlink answer reporting 0");uint256[] memory normalizedWeights =IManagedPool(address(balancerPool)).getNormalizedWeights();uint256 amount = (((_alcxAmount *uint256(alcxEthPrice)) /1ether) * normalizedWeights[0]) / normalizedWeights[1];return (amount, normalizedWeights); }
Test 1: running the test with the 60 days stale threshold active, and making sure that the priceTimestamp is 59 days old, as can see from above modifications:
The test completed successfully, so a very stale price of 59 days old was used, and the system accepted it, which should have been an invalid price due to extreme staleness.
Test 2: now running the test with the correct 30 days stale threshold value, and keeping everything else same as for above test, should now revert as expected:
reverted as expected because of stale price over 30 days
Suggested bugfix:
function amountToCompound(uint256 _alcxAmount) public view returns (uint256, uint256[] memory) { // Increased for testing since tests go into future- uint256 staleThreshold = 60 days;+ uint256 staleThreshold = 30 days;
