Olympus DAO contest - pashov's results

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/modules/TRSRY.sol#L64

Vulnerability details

Proof of concept

The problem is perfectly described here https://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/edit

The tldr; is that if you approved a person to spend 100 tokens and then you want to decrease his allowance to 50, if he spends his 100 tokens allowance before you set his allowance to 50 he will be able to spend 50 more. This results in him being able to spend 150 tokens instead of your desired 50 allowance.

Impact

This can leads to much more token amount being transferred from a user’s wallet than what he wants, since if he is decreasing the allowance he would want less tokens to be spent from his wallet, but this way actually more tokens could be spent.

Recommendation

Add OpenZeppelin’s safeIncreaseAllowance and safeDecreaseAllowance functionality to TRSRY.sol. You can use this SafeERC20.sol implementation as a reference https://github.com/OpenZeppelin/openzeppelin-contracts/blob/3dac7bbed7b4c0dbf504180c33e8ed8e350b93eb/contracts/token/ERC20/utils/SafeERC20.sol

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/modules/PRICE.sol#L161 https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/modules/PRICE.sol#L170

Vulnerability details

Proof of Concept

(, int256 ohmEthPriceInt, , uint256 updatedAt, ) = _ohmEthPriceFeed.latestRoundData();
            // Use a multiple of observation frequency to determine what is too old to use.
            // Price feeds will not provide an updated answer if the data doesn't change much.
            // This would be similar to if the feed just stopped updating; therefore, we need a cutoff.
            if (updatedAt < block.timestamp - 3 * uint256(observationFrequency))
                revert Price_BadFeed(address(_ohmEthPriceFeed));
            ohmEthPrice = uint256(ohmEthPriceInt);

            int256 reserveEthPriceInt;
            (, reserveEthPriceInt, , updatedAt, ) = _reserveEthPriceFeed.latestRoundData();
            if (updatedAt < block.timestamp - uint256(observationFrequency))
                revert Price_BadFeed(address(_reserveEthPriceFeed));
            reserveEthPrice = uint256(reserveEthPriceInt);

For both _ohmEthPriceFeed and _reserveEthPriceFeed the validation of the result when calling latestRoundData is insufficient. The code has a correct value check for the updatedAt value, but it is missing one to check if price is actually a positive number and also to check the answeredInRound return value to see if the price is stale.

Impact

The PRICE.sol contract and logic are a core part of the protocol and using a zero or a stale price can result in the protocol returning an incorrect moving average or a zero one. This means the prices in the Range module will be set incorrectly.

Recommendation

Change the latestRoundData logic to the following (you can use custom errors instead require statements as well):

(roundId, rawPrice,, updatedAt, answeredInRound) = feedVariable.latestRoundData()
require(rawPrice > 0, "Chainlink price <= 0");
require(answeredInRound >= roundId, "Stale price");

Change the code exactly the same way for both latestRoundData calls, and also leave the current check for updatedAt.