[Audit Report] Olympus

Scope

---

Files	SLOC	Coverage
Kernel Contracts (2)
src/Kernel.sol Σ	262	100.00%
src/utils/KernelUtils.sol 🖥 Σ	46	-
Module Contracts (6)
src/modules/TRSRY.sol Σ	90	100.00%
src/modules/MINTR.sol	21	100.00%
src/modules/RANGE.sol	224	100.00%
src/modules/PRICE.sol Σ	145	100.00%
src/modules/VOTES.sol Σ	38	100.00%
src/modules/INSTR.sol Σ	44	100.00%
Policy Contracts (7)
src/policies/TreasuryCustodian.sol Σ	56	100.00%
src/policies/Operator.sol	487	100.00%
src/policies/BondCallback.sol Σ	122	100.00%
src/policies/Heart.sol	82	100.00%
src/policies/PriceConfig.sol	41	100.00%
src/policies/Governance.sol Σ	183	100.00%
src/policies/VoterRegistration.sol	28	100.00%
Interfaces (3)
src/interfaces/IBondCallback.sol	11	-
src/policies/interfaces/IHeart.sol	10	-
src/policies/interfaces/IOperator.sol	54	-
Total (18 files):	1944	100.00%

내가 생각했던 결함들

---

length == 0 위치 수정

//INSTR.sol

    function store(Instruction[] calldata instructions_) external permissioned returns (uint256) {
        uint256 length = instructions_.length;
        uint256 instructionsId = ++totalInstructions;

        Instruction[] storage instructions = storedInstructions[instructionsId];

        if (length == 0) revert INSTR_InstructionsCannotBeEmpty();

        for (uint256 i; i < length; ) {
            Instruction calldata instruction = instructions_[i];
            ensureContract(instruction.target);

            // If the instruction deals with a module, make sure the module has a valid keycode (UPPERCASE A-Z ONLY)
            if (
                instruction.action == Actions.InstallModule ||
                instruction.action == Actions.UpgradeModule
            ) {
                Module module = Module(instruction.target);
                ensureValidKeycode(module.KEYCODE());
            } else if (instruction.action == Actions.ChangeExecutor && i != length - 1) {
                // Throw an error if ChangeExecutor exists and is not the last Action in the instruction list.
                // This exists because if ChangeExecutor is not the last item in the list of instructions,
                // the Kernel will not recognize any of the following instructions as valid, since the policy
                // executing the list of instructions no longer has permissions in the Kernel. To avoid this issue
                // and prevent invalid proposals from being saved, we perform this check.
                revert INSTR_InvalidChangeExecutorAction();
            }

            instructions.push(instructions_[i]);
            unchecked {
                ++i;
            }
        }

        emit InstructionsStored(instructionsId);

        return instructionsId;
    }

instructions_을 받아서 기존 instructions에 추가하는 함수다. 그런데 instructions_을 받을 때 들어온 값의 length가 0인지 체크하는 부분이 처음이 아니라 중간에 있다. revert가 나서 크게 상관없겠지만, 초반에 확인하는 게 좋지 않을까 싶어서 가져와봤다.

observationFrequency_이 0이면 현재 가격을 가져올 수 없음

//PRICE.sol

    constructor(
        Kernel kernel_,
        AggregatorV2V3Interface ohmEthPriceFeed_,
        AggregatorV2V3Interface reserveEthPriceFeed_,
        uint48 observationFrequency_,
        uint48 movingAverageDuration_
    ) Module(kernel_) {
        /// @dev Moving Average Duration should be divisible by Observation Frequency to get a whole number of observations
        if (movingAverageDuration_ == 0 || movingAverageDuration_ % observationFrequency_ != 0)
            revert Price_InvalidParams();
	//..
    }


    function getCurrentPrice() public view returns (uint256) {
        if (!initialized) revert Price_NotInitialized();

        // Get prices from feeds
        uint256 ohmEthPrice;
        uint256 reserveEthPrice;
        {
            (, 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);
        }

        // Convert to OHM/RESERVE price
        uint256 currentPrice = (ohmEthPrice * _scaleFactor) / reserveEthPrice;

        return currentPrice;
    }

처음 constructor에서 movingAverageDuration_과 observationFrequency_ 값을 넣어준다. 그런데 if문을 통해 movingAverageDuration_ 값만 0인지 확인한다. constructor에서는 상관이 없지만 이후 getCurrentPrice()에서 아래 if문 때문에 계속 revert가 날 것이다. updatedAt은 무조건 현재 block.timestamp 보다 작을 수 밖에 없기 때문이다.

if (updatedAt < block.timestamp - 3 * uint256(observationFrequency))
                revert Price_BadFeed(address(_ohmEthPriceFeed));

따라서 constructor 부분에 observationFrequency_ 값도 0인지 확인하는 부분이 추가되어야 한다.

새로 배운 것

1. type C is V

//Kernel.sol

type Keycode is bytes5;
type Role is bytes32;



//KernelUtils.sol

function toKeycode(bytes5 keycode_) pure returns (Keycode) {
    return Keycode.wrap(keycode_);
}

// solhint-disable-next-line func-visibility
function fromKeycode(Keycode keycode_) pure returns (bytes5) {
    return Keycode.unwrap(keycode_);
}

KernelUtils에 .wrap()과 .unwrap()이 있는데 Kernel이나 다른 컨트랙트를 아무리 찾아 봐도 해당 함수를 정의하는 부분은 없었다. bytes 타입에 속해있는 타입인지 찾아봤지만 별 내용이 없었다. 계속 공식 문서를 뒤지다가 아래 내용을 발견했다.

사용자가 지정한 타입(C)과 기존에 존재하는 타입(V)을 조절할 수 있다. type C is V을 예로 들면, C.wrap(a)를 했을 때 원래 V타입인 a가 C타입으로 변하고, C.unwrap(a)를 하면 다시 V타입으로 돌아온다.

2. EXTCODESIZE Checks

//KernelUtils.sol

function ensureContract(address target_) view {
    uint256 size;
    assembly {
        size := extcodesize(target_)
    }
    if (size == 0) revert TargetNotAContract(target_);
}

target이 컨트랙트인지 아닌지 확인하는 함수다. 왜 extcodesize를 확인해서 0이 아니어야 하는지 궁금해서 찾아봤다. Consensys와 ethereum stackexchange 답변에 따르면 extcodesize는 해당 계정의 코드 길이를 리턴한다. 즉, 코드를 포함한 컨트랙트라면 코드가 존재하기 때문에 0보다 큰 값을 리턴할 것이고 메타마스크 같은 EOA라면 코드가 존재하지 않기 때문에 0을 리턴할 것이다.

한 가지 주의할 점은 컨트랙트의 constructor에서 함수를 호출할 경우 컨트랙트가 배포 될 시점의 extcodesize는 0이기 때문에 이 점을 이용해서 공격당할 수 있다는 점이다. 따라서 보안에 취약한 부분에 사용하지 않는 것을 추천한다.

3. unchecked

    instructions.push(instructions_[i]);
    unchecked {
        ++i;
    }

가끔 함수 마지막 부분에 unchecked가 있다. 무슨 용도인지 찾아봤다. 원래 underflow/overflow를 방지하기 위해 솔리디티 자체에서 검사를 하는데 이 때 가스가 소모된다. underflow/overflow가 일어날 일이 없다면 unchecked를 사용해서 가스를 절약할 수 있다고 한다.

레포트

---

[H-01] IN GOVERNANCE.SOL, IT MIGHT BE IMPOSSIBLE TO ACTIVATE A NEW PROPOSAL FOREVER AFTER FAILED TO EXECUTE THE PREVIOUS ACTIVE PROPOSAL.

//GOVERNANCE.sol

function activateProposal(uint256 proposalId_) external {
        ProposalMetadata memory proposal = getProposalMetadata[proposalId_];

        if (msg.sender != proposal.submitter) {
            revert NotAuthorizedToActivateProposal();
        }

        if (block.timestamp > proposal.submissionTimestamp + ACTIVATION_DEADLINE) {
            revert SubmittedProposalHasExpired();
        }

        if (
            (totalEndorsementsForProposal[proposalId_] * 100) <
            VOTES.totalSupply() * ENDORSEMENT_THRESHOLD
        ) {
            revert NotEnoughEndorsementsToActivateProposal();
        }

        if (proposalHasBeenActivated[proposalId_] == true) {
            revert ProposalAlreadyActivated();
        }

        if (block.timestamp < activeProposal.activationTimestamp + GRACE_PERIOD) {
            revert ActiveProposalNotExpired();
        }

        activeProposal = ActivatedProposal(proposalId_, block.timestamp);

        proposalHasBeenActivated[proposalId_] = true;

        emit ProposalActivated(proposalId_, block.timestamp);
    }

proposal을 active상태로 만들기 위해선 여러 조건들이 필요하다. 그 중에 문제가 발생한 조건은 다음과 같다.

		if (
            (totalEndorsementsForProposal[proposalId_] * 100) <
            VOTES.totalSupply() * ENDORSEMENT_THRESHOLD
        ) {
            revert NotEnoughEndorsementsToActivateProposal();
        }

솔직히 말하면 나는 해당 조건들을 볼 때 그 조건들이 무엇을 의미하는지 정확하게 파악하지 못했다. VOTES.totalSupply() * ENDORSEMENT_THRESHOLD가 왜 있는지 고민하다가 넘겼었다. 그런데 다시 생각해보니 위 조건문을 다음과 같이 바꿀 수 있다.

		if (
            (totalEndorsementsForProposal[proposalId_] / VOTES.totalSupply()) <
             ENDORSEMENT_THRESHOLD / 100
        ) {
            revert NotEnoughEndorsementsToActivateProposal();
        }

즉, 전체 투표 수 중에서 사람들이 해당 프로포절에 투표한 비율이 20%를 넘지 못하면 active 할 수 없다는 것이다. 이 말은 사람들이 가지고 있는 투표수가 전체의 20퍼센트가 최소한 있어야 한다는 말과 같다.

만약 하나의 프로포절에 81퍼센트의 투표가 몰리면 어떻게 될까?

문제는 다음과 같은 상황에서 발생한다. 하나의 프로포절에 찬성 43%, 반대 37%로 전체 투표의 81%가 몰렸다. 그렇다면 현재 유저들이 가지고 있는 투표수는 19%로 ENDORSEMENT_THRESHOLD / 100를 넘지 못 한다. 따라서 현재 activate된 프로포절이 가결/부결되기 전까지 새로운 프로포절은 active 될 수 없다. 또한 executeProposal()에도 해당 조건문이 포함되어 있어서 execute할 때에도 20%가 필요하다. 그래서 프로포절이 active 상태에 계속 머물 수 밖에 없다.

warden이 제안한 대안은 EXECUTION_EXPIRE를 constant로 해서 2주로 잡는 것이다. 2주가 지나면 투표자들이 자신의 투표를 reclaim할 수 있도록 해서 다시 투표권을 가져가도록 한다. 결국 투표권이 묶이는 것을 방지함으로써 문제를 해결할 수 있다.

[H-02] ANYONE CAN PASS ANY PROPOSAL ALONE BEFORE FIRST VOTES ARE MINTED

//GOVERNANCE.sol

    function submitProposal(
        Instruction[] calldata instructions_,
        bytes32 title_,
        string memory proposalURI_
    ) external {
        if (VOTES.balanceOf(msg.sender) * 10000 < VOTES.totalSupply() * SUBMISSION_REQUIREMENT)
            revert NotEnoughVotesToPropose();
		//..
        emit ProposalSubmitted(proposalId);
    }

    function activateProposal(uint256 proposalId_) external {
		//...
        if (
            (totalEndorsementsForProposal[proposalId_] * 100) <
            VOTES.totalSupply() * ENDORSEMENT_THRESHOLD
        ) {
            revert NotEnoughEndorsementsToActivateProposal();
        }
        //..
    }

    function executeProposal() external {
        uint256 netVotes = yesVotesForProposal[activeProposal.proposalId] -
            noVotesForProposal[activeProposal.proposalId];
        if (netVotes * 100 < VOTES.totalSupply() * EXECUTION_THRESHOLD) {
            revert NotEnoughVotesToExecute();
        }

        if (block.timestamp < activeProposal.activationTimestamp + EXECUTION_TIMELOCK) {
            revert ExecutionTimelockStillActive();
        }
		//..
    }

프로포절이 통과되기까지의 전체 순서는 submit - activate - execute이다. 각 단계별로 함수가 실행되기 위한 조건이 있다. 이 때 공통적으로 VOTES.totalSupply()의 가중치보다 특정 값이 작으면 revert가 나게 되어있다.

만약 VOTES.totalSupply()가 0이라면?

VOTES가 민팅되기 전이라면 VOTES.totalSupply()는 0이다. 이 때 각 함수의 조건을 다시 보면, 놀랍게도 모두 false가 되면서 revert를 벗어날 수 있다. 즉, 투표권 없이 프로포절을 submit - activate - execute 모두 성공시킬 수 있다는 얘기다. 이는 자연스럽게 자금 탈취로 이어질 것이고 프로젝트에 손해를 끼칠 것이다.

만약 콘테스트 중에 위와 같은 생각을 떠올렸다면 바로 테스트 코드를 작성해야 한다. 아래는 warden이 이슈와 함께 제출한 테스트 코드다. VOTES가 민팅되기 전에 Admin과 Executor 권한을 넘기는 프로포절을 올리고 execute 한 다음 성공적으로 권한 탈취한 것 까지 나와있다.

    function test_AttackerPassesProposalBeforeMinting() public {
        //자체적으로 만든 user. vm.deal()을 통해 100이더 보유
        address[] memory users = userCreator.create(1);
        address attacker = users[0];
      
        //atacker 연결
        vm.prank(attacker);
      
        //컨트랙트와 프로포절 올릴 instruction 생성
        MockMalicious attackerControlledContract = new MockMalicious();
        Instruction[] memory instructions_ = new Instruction[](2);
        
        //struct Instruction(action,target)
        instructions_[0] = Instruction(Actions.ChangeAdmin, address(attackerControlledContract));
        instructions_[1] = Instruction(Actions.ChangeExecutor, address(attackerControlledContract));
      
        vm.prank(attacker);
        governance.submitProposal(instructions_, "proposalName", "This is the proposal URI")
        governance.endorseProposal(1);
        
        vm.prank(attacker);
        governance.activateProposal(1);
        
        vm.warp(block.timestamp + 3 days + 1);
        
        governance.executeProposal();
       
        //권한 탈취 성공
        assert(kernel.executor()==address(attackerControlledContract));
        assert(kernel.admin()==address(attackerControlledContract));
    }

이를 방지하기 위해 각 함수에 VOTES.totalSupply() 의 최솟값을 확인하는 코드를 추가해야 한다.

[H-03] TRSRY: FRONT-RUNNABLE SETAPPROVALFOR

//TRSRY.sol

    function setApprovalFor(
        address withdrawer_,
        ERC20 token_,
        uint256 amount_
    ) external permissioned {
        withdrawApproval[withdrawer_][token_] = amount_;

        emit ApprovedForWithdrawal(withdrawer_, token_, amount_);
    }

//TreasuryCustodian.sol

    function grantApproval(
        address for_,
        ERC20 token_,
        uint256 amount_
    ) external onlyRole("custodian") {
        TRSRY.setApprovalFor(for_, token_, amount_);
    }

기존에 Alice는 100만큼 인출할 수 있다. 만약 custodian이 Alice의 인출 한도를 50으로 줄이는 트랜잭션을 보낸다고 가정 했을 때, mempool을 모니터링 하고 있던 Alice는 가스비를 더 줘서 1) 먼저 100만큼 인출하고 2) 한도가 50으로 줄어들면 3) 다시 50을 새로 인출할 수 있다. 이를 방지하기 위해선 approve 하는 금액을 새로 할당하는 것이 아닌 기존 인출가능 금액에서 차감하는 형식으로 코드를 수정해야 한다.

withdrawApproval 관련 함수가 나오면 항상 mempool을 모니터링 하고 있는 Alice를 기억하자.

[M-05] PROPOSALS OVERWRITE

//Governance.sol

    function submitProposal(
        Instruction[] calldata instructions_,
        bytes32 title_,
        string memory proposalURI_
    ) external {
        if (VOTES.balanceOf(msg.sender) * 10000 < VOTES.totalSupply() * SUBMISSION_REQUIREMENT)
            revert NotEnoughVotesToPropose();

        uint256 proposalId = INSTR.store(instructions_);
        getProposalMetadata[proposalId] = ProposalMetadata(
            title_,
            msg.sender,
            block.timestamp,
            proposalURI_
        );

        emit ProposalSubmitted(proposalId);
    }

INSTR.store(instructions_)을 통해 getProposalMetadata[proposalId]에 새로운 프로포절을 할당할 때 이미 있는 프로포절인지 확인하지 않는다. 따라서 이전 프로포절이 누락되고 새로운 프로포절로 대체될 수 있다. 중복 체크하는 코드가 추가되어야 한다.

[M-06] AFTER ENDORSING A PROPOSAL, USER CAN TRANSFER VOTES TO ANOTHER USER FOR ENDORSING THE SAME PROPOSAL AGAIN

/// @notice Votes module is the ERC20 token that represents voting power in the network.
/// @dev    This is currently a substitute module that stubs gOHM.
contract OlympusVotes is Module, ERC20 {
  //..

코멘트에 OlympusVotes는 gOHM의 대체제로 사용된다고 나와있다. that stubs gOHM이라는 말은 현재 테스트 단계에서 직접적으로 gOHM을 사용할 수 없으니 gOHM을 본격적으로 사용하기 전에 임시적으로 OlympusVotes를 사용할 것이고, 추후에 gOHM으로 바꾼다는 뜻이다.

As an additional safety mechanism, when votes are cast, the tokens used to vote on the proposal are locked in the contract until the proposal is no longer active. This exists to deter malicious behavior by ensuring users cannot transfer their voting tokens until after the proposal has been resolved. Once a proposal is no longer active, either by being executed or replaced by a new proposal, users may redeem their votes back from the contract.

프로젝트 측의 공식문서에 따르면 Votes 토큰은 투표 직후 이동할 수 없다. 그러나 현재 이더스캔에서 gOHM의 컨트랙트를 보면 transfer와 transferFrom 함수가 존재한다. 따라서 추후에 gOHM으로 변경했을 때 해커가 투표 직후 다른 지갑으로 gOHM을 전송해서 중복 투표할 수 있다.

이를 예방하기 위한 방법으로는 유저들이 투표했을 때 해당 토큰을 거버넌스에 종속되도록 해서 다른 지갑으로 전송하지 못 하게 하는 것이다. 해당 이슈가 제출되고 나서 프로젝트 측에서는 staking vault를 통해 방지할 예정이라고 한다.

피드백

---

1. 조건문과 숫자가 나온다면 주의 깊게 볼 것. 이해가 잘 안 되면 공식을 변형해서 이해해 볼 것.
2. 자금이 빠져나가는 것만 High vulnerability가 아니라, 프로젝트가 의도된 방향으로 운영되지 않는 것도 High vulnerability다.
3. 어떤 일을 위해 토큰이 필요하다면, 토큰이 민팅되기 전의 상황을 생각해 볼 것.
4. withdrawApproval 패턴 - frontRun 기억하기
5. 코멘트에 이슈와 관련된 힌트가 있을 수 있다.
6. HM 이슈들은 컨트랙트의 메인 로직에서 주로 발견된다. 컨트랙트의 메인이 어떤 부분인지 먼저 파악하자.

출처 및 참고자료

---

1. EXTCODESIZE Checks
2. How does a contract find out if another address is a contract?