Olympus DAO contest - brgltd's results

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/libraries/TransferHelper.sol#L10-L33

Vulnerability details

Impact

TransferHelper.sol and solmate won't check if the token is a contract or not.

A hacker could set traps for non existing tokens to steal future funds from users.

Proof of Concept

The safeTransfer() functions used in the contract are wrappers around the solmate library. Solmate will not check for contract existance.

File: src/libraries/TransferHelper.sol
function safeTransferFrom(
    ERC20 token,
    address from,
    address to,
    uint256 amount
) internal {
    (bool success, bytes memory data) = address(token).call(
        abi.encodeWithSelector(ERC20.transferFrom.selector, from, to, amount)
    );

    require(success && (data.length == 0 || abi.decode(data, (bool))), "TRANSFER_FROM_FAILED");
}

function safeTransfer(
    ERC20 token,
    address to,
    uint256 amount
) internal {
    (bool success, bytes memory data) = address(token).call(
        abi.encodeWithSelector(ERC20.transfer.selector, to, amount)
    );

    require(success && (data.length == 0 || abi.decode(data, (bool))), "TRANSFER_FAILED");
}

Tools Used

Manual review.

Recommended Mitigation Steps

Looking at OpenZeppelin's implementation, a check is made on the code for the token address.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L36-L42

address(token).code.length > 0

Consider using OpenZeppelin or checking for contract existance manually inside TransferHelper.sol.

Lines of code

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

Vulnerability details

Impact

Not checking additional fields returned by Chainlink might cause incorrect prices being processed.

Proof of Concept

The only values being check from latestRoundData are price and updatedAt.

File: src/modules/PRICE.sol
161: (, int256 ohmEthPriceInt, , uint256 updatedAt, ) = _ohmEthPriceFeed.latestRoundData();
170: (, reserveEthPriceInt, , updatedAt, ) = _reserveEthPriceFeed.latestRoundData();

Recommened Mitigation Steps

Add additional checks for roundId and answeredInRound. Also, add a check to ensure updatedAt is not zero.

$ git diff --no-index PRICE.sol.orig PRICE.sol
diff --git a/PRICE.sol.orig b/PRICE.sol
index 55d85d3..b0792ea 100644
--- a/PRICE.sol.orig
+++ b/PRICE.sol
@@ -158,18 +158,30 @@ contract OlympusPrice is Module {
         uint256 ohmEthPrice;
         uint256 reserveEthPrice;
         {
-            (, int256 ohmEthPriceInt, , uint256 updatedAt, ) = _ohmEthPriceFeed.latestRoundData();
+            (uint256 roundId, int256 ohmEthPriceInt, , uint256 updatedAt, uint256 answeredInRound) = _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));
+            // Example of a new custom error to handle stale prices.
+            if (answeredInRound < roundId)
+                revert Price_Stale();
+            // Example of a new custom error to handle incomplete rounds.
+            if (updatedAt == 0)
+                revert Price_IncompleteRound();
             ohmEthPrice = uint256(ohmEthPriceInt);

             int256 reserveEthPriceInt;
-            (, reserveEthPriceInt, , updatedAt, ) = _reserveEthPriceFeed.latestRoundData();
+            (uint256 roundId, reserveEthPriceInt, , updatedAt, uint256 answeredInRound) = _reserveEthPriceFeed.latestRoundData();
             if (updatedAt < block.timestamp - uint256(observationFrequency))
                 revert Price_BadFeed(address(_reserveEthPriceFeed));
+            // Example of a new custom error to handle stale prices.
+            if (answeredInRound < roundId)
+                revert Price_Stale();
+            // Example of a new custom error to handle incomplete rounds.
+            if (updatedAt == 0)
+                revert Price_IncompleteRound();
             reserveEthPrice = uint256(reserveEthPriceInt);
         }

[L-01] Missing nonReentrant for function not using checks-effects-interactions

The batchToTresury function has access control, but it's updating the state after external calls. Consider adding a nonReetrancy modifier.

File: src/policies/BondCallback.sol
function batchToTreasury(ERC20[] memory tokens_) external onlyRole("callback_admin") {
    ERC20 token;
    uint256 balance;
    uint256 len = tokens_.length;
    for (uint256 i; i < len; ) {
        token = tokens_[i];
        balance = token.balanceOf(address(this));
        token.safeTransfer(address(TRSRY), balance);
        priorBalances[token] = token.balanceOf(address(this));

        unchecked {
            ++i;
        }
    }
}

[L-02] Missing zero address checks for setters

Consider adding checks against zero address when a function is receiving an input address.

File: main/src/modules/TRSRY.sol
64: function setApprovalFor(
122: function setDebt(

[NC-01] Non library files should use fixed compiler verion

Locking the pragma helps to ensure that contracts do not accidentally get deployed using an outdated compiler version. Note that pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or a package.

There are 3 instances of this issue.

File: src/interfaces/IBondCallback.sol
2: pragma solidity >=0.8.0;

File: src/policies/interfaces/IHeart.sol
2: pragma solidity >=0.8.0;

File: src/policies/interfaces/IOperator.sol
2: pragma solidity >=0.8.0;

[NC-02] Public functions not called by the contract should be declared external

There are 2 instances of this issue

File: src/policies/Governance.sol
151: function getActiveProposal() public view returns (ActivatedProposal memory) {
145: function getMetadata(uint256 proposalId_) public view returns (ProposalMetadata memory) {

[NC-03] Missing NATSPEC

Consider adding NATSPEC on all functions to enhance the project documentation.

File: src/policies/Governance.sol
61: function configureDependencies() external override returns (Keycode[] memory dependencies) {
70: function requestPermissions()

[NC-04] Lack of event when kernel grants or revoke status

Consider emitting an event when setActiveStatus is called to facilitate monitoring of the system.

File: main/src/modules/TRSRY.sol
126: function setActiveStatus(bool activate_) external onlyKernel {

[NC-05] TODOs should should be resolved before deployment

There are 4 instances of this issue.

File: src/policies/Operator.sol
657: /// TODO determine if this should use the last price from the MA or recalculate the current price, ideally last price is ok since it should have been just updated and should include check against secondary?

File: src/policies/TreasuryCustodian.sol
51: // TODO Currently allows anyone to revoke any approval EXCEPT activated policies.
52: // TODO must reorg policy storage to be able to check for deactivated policies.
56: // TODO Make sure `policy_` is an actual policy and not a random address.

[G-01] Prefix increment costs less gas than postfix increment

There are 7 instances of this issue.

File: src/policies/Operator.sol
488: decimals++;
670: _status.low.count++;
675: _status.low.count--;
686: _status.high.count++;
691: _status.high.count--;

File: src/utils/KernelUtils.sol
49: i++;
64: i++;

[G-02] Cache the length of the array before the loop

There is 1 instance of this issue.

File: src/policies/Governance.sol
278: for (uint256 step; step < instructions.length; ) {

[G-03] Initializing a variable with the default value wastes gas

There are 3 instances of this issue.

File: src/Kernel.sol
397: for (uint256 i = 0; i < reqLength; ) {

File: src/utils/KernelUtils.sol
43: for (uint256 i = 0; i < 5; ) {
58: for (uint256 i = 0; i < 32; ) {

[G-04] Use != 0 instead of > 0 to save gas.

Replace > 0 with != 0 for unsigned integers.

On the instance bellow userVotesForProposal is a nested mapping of uints.

File: src/policies/Governance.sol
247: if (userVotesForProposal[activeProposal.proposalId][msg.sender] > 0) {

[G-05] Use right/left shift instead of division/multiplication to save gas

There are 5 instances of this issue.

File: src/policies/Operator.sol
372: int8 scaleAdjustment = int8(ohmDecimals) - int8(reserveDecimals) + (priceDecimals / 2);
419: uint256 invCushionPrice = 10**(oracleDecimals * 2) / range.cushion.low.price;
420: uint256 invWallPrice = 10**(oracleDecimals * 2) / range.wall.low.price;
427: int8 scaleAdjustment = int8(reserveDecimals) - int8(ohmDecimals) + (priceDecimals / 2);
786: ) * (FACTOR_SCALE + RANGE.spread(true) * 2)) /

[G-06] Don’t compare boolean expressions to boolean literals

There are 2 instances of this issue.

File: src/policies/Governance.sol
223: if (proposalHasBeenActivated[proposalId_] == true) {
306: if (tokenClaimsForProposal[proposalId_][msg.sender] == true) {

[G-07] x += y costs more gas than x = x + y for state variables

There is 1 instance of this issue in Price.sol and 2 instances in TRSRY.sol

File: src/modules/PRICE.sol
136: _movingAverage += (currentPrice - earliestPrice) / numObs;

File: main/src/modules/TRSRY.sol
96: reserveDebt[token_][msg.sender] += amount_;
97: totalDebt[token_] += amount_;

[G-08] Using private rather than public for constants, saves gas

The values can still be inspected on the source code if necessary.

There are 9 instances of this issue.

File: src/modules/PRICE.sol
59: uint8 public constant decimals = 18;

File: src/modules/RANGE.sol
65: uint256 public constant FACTOR_SCALE = 1e4;

File: src/policies/Governance.sol
121: uint256 public constant SUBMISSION_REQUIREMENT = 100;
124: uint256 public constant ACTIVATION_DEADLINE = 2 weeks;
127: uint256 public constant GRACE_PERIOD = 1 weeks;
130: uint256 public constant ENDORSEMENT_THRESHOLD = 20;
133: uint256 public constant EXECUTION_THRESHOLD = 33;
137: uint256 public constant EXECUTION_TIMELOCK = 3 days;

File: src/policies/Operator.sol
89: uint32 public constant FACTOR_SCALE = 1e4;