Olympus DAO contest - csanuragjain's results

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L295

Vulnerability details

Impact

Due to insufficient checks User will not be able to withdraw there Votes after a non successful proposal.

1. The effect could be permanent if majority vote holders participated in the proposal, such that new proposal can't be created due to noone having votes to reach threshold for creating proposal

2. Since User will not be able to withdraw till new proposal is endorsed, users will start endorsing the upcoming proposal even they dont want to, just to get a new active proposal and get back there Votes stuck in system

Proof of Concept

1. Proposal id 1 is currently active for Voting

2. Voting completes and below is outcome of Voting:

Yes Votes : 50
No Votes : 60

3. Since No Votes are higher so proposal cannot execute.

4. User wants to reclaim back there "Votes" using reclaimVotes function

5. This fails since this proposal is still the active proposal

if (proposalId_ == activeProposal.proposalId) {
            revert CannotReclaimTokensForActiveVote();
        }

6. So User has to wait until some user creates a new proposal using submitProposal function and proposal is activated to change the active proposal to this new proposal

function activateProposal(uint256 proposalId_) external {
...
activeProposal = ActivatedProposal(proposalId_, block.timestamp);
...
}

7. Now the problem here is:

a. The problem could be permanent if majority vote holders participated in the proposal, such that new proposal can't be created due to noone having votes to reach threshold for creating proposal

b. Since User will not be able to withdraw till new proposal is endorsed, users will start activating the upcoming proposal even they dont want to, just to get a new active proposal and get back there Votes stuck in system

Recommended Mitigation Steps

User should be allowed to "reclaimVotes" once execution window is closed

function reclaimVotes(uint256 proposalId_) external {

if (proposalId_ == activeProposal.proposalId   ||   block.timestamp < activeProposal.activationTimestamp + EXECUTION_TIMELOCK) {
            revert CannotReclaimTokensForActiveVote();
        }

}

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/TreasuryCustodian.sol#L42

Vulnerability details

Impact

Frontrunning is possible where user withdraw money before setApproval refill the approval limit. This allows User to get more funds than required

Proof of Concept

1. User A is approved an amount of 50

2. "custodian" decides to reduce the approval amount to 20 for which he uses grantApproval

3. User A realizes this and frontrun custodian request by withdrawing 50 amount

4. Post that custodian request executes which again sets approval to 20 which means User A can withdraw 20 more amount

5. So User A can withdraw 50+20 = 70 amount when ideally only 20 amount should have been withdrawn

Recommended Mitigation Steps

Set approval to 0 first so that Admin can later update the approval (if User A already took out 50 amount, Admin can refrain from setting the approval again)

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L180

Vulnerability details

Impact

Attacker can fake endorse a proposal by transferring Votes token from one account to another (Dependency: Need a policy which allows arbitrary transferFrom). Since endorsement is not locking the Votes token (like in vote function), so attack is easy to perform

Proof of Concept

1. Proposal id 1 is submitted

2. User A endorse this proposal with his 50k Votes

function endorseProposal(uint256 proposalId_) external {
        uint256 userVotes = VOTES.balanceOf(msg.sender);

        ...

        userEndorsementsForProposal[proposalId_][msg.sender] = userVotes;
        ...
    }

3. Assume a policy P which is approved allows users to transfer Votes from one account to another

4. User uses this policy to immediately transfer all Votes from his account to another account B

5. User use account B to again endorse the proposal which works

Recommended Mitigation Steps

Like in Vote function, once user endorse a proposal, user's votes should be transferred to the contract and the number of Votes used in endorsement can then be utilized by vote function

function endorseProposal(uint256 proposalId_) external {
...
userEndorsementsForProposal[proposalId_][msg.sender] = userVotes;
VOTES.transferFrom(msg.sender, address(this), userVotes);
...
}

function vote(bool for_) external {
uint256 userVotes = VOTES.balanceOf(msg.sender) + userEndorsementsForProposal[proposalId_][msg.sender] ;
}

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L240

Vulnerability details

Impact

Once user has voted, he is not allowed to vote again even if buys more token which is unfair. Assume voting is in last stage and user who already voted want to influence the votes is not able to vote

Proof of Concept

1. User A votes YES on proposal with his 50k votes

2. Proposal is in its last stage and currently losing by 2k Votes

3. User A wants this proposal to win but even if he buys extra 2-3k votes, Vote function is just one time operation and User A cannot change the votes number now

Recommended Mitigation Steps

Allow user to revote like its done in endorsing a proposal

Lines of code

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

Vulnerability details

Impact

Additional checks on latestRoundData is missing which means price returned by getCurrentPrice function might be incorrect

Recommended Mitigation Steps

Consider validating the output of latestRoundData

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

        // Get prices from feeds
        uint256 ohmEthPrice;
        uint256 reserveEthPrice;
        {
            (uint80 roundID, int256 ohmEthPriceInt, , uint256 updatedAt, uint80 answeredInRound) = _ohmEthPriceFeed.latestRoundData();
require(
          answeredInRound >= roundID,
          "Price Stale"
      );
      require(ohmEthPriceInt > 0, "Malfunction");
      require(updatedAt != 0, "Incomplete round");
}

Activated proposal can still be endorsed

Contract: https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L180

Function: endorseProposal

Issue: It was observed that even though a proposal is activated, user can still endorse it

Recommendation: Do not allow endorsing once proposal is activated

Zero address check missing

Contract: https://github.com/code-423n4/2022-08-olympus/blob/main/src/Kernel.sol#L76

Function: changeKernel

Issue: It was observed that new kernel can be set as zero address which will prohibit all kernel activities

Recommendation: Do not allow zero address while setting kernel