Olympus DAO contest - yixxas's results

Lines of code

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

Vulnerability details

Impact

Submission of proposal requires 1% of VOTES of total supply, whereas endorsement of proposal requires 20% of VOTES of total supply ( default values ). However, users are only required to temporarily transfer their VOTES to the protocol when they are casting their votes and not during the submission or endorsement of proposal phase. This means that the endorsement layer of attempting to prevent and reduce spam can effectively be circumvented. Any users with enough VOTES to submit a proposal will be able to endorse it making endorsement an ineffective measure.

This can potentially lead to much more spam proposals to the protocol as the number of votes required is 20x lower.

Proof of Concept

When the same address calls the endorseProposal() function twice, it updates only the latest votes. However, there is nothing preventing the user from transferring their VOTES token to another address after endorsing and call this same function. The same VOTES can be used to increase totalEndorsementsForProposal count infinitely.

Governance.sol#L180-L201

    function endorseProposal(uint256 proposalId_) external {
        ...
        // undo any previous endorsement the user made on these instructions
        uint256 previousEndorsement = userEndorsementsForProposal[proposalId_][msg.sender];
        totalEndorsementsForProposal[proposalId_] -= previousEndorsement;

        // reapply user endorsements with most up-to-date votes
        userEndorsementsForProposal[proposalId_][msg.sender] = userVotes;
        totalEndorsementsForProposal[proposalId_] += userVotes;

        emit ProposalEndorsed(proposalId_, msg.sender, userVotes);
    }

Recommended Mitigation Steps

Consider taking a snapshot of the number of tokens a user has in the same block as when the proposal is made. This means that whatever transfers being made after this block number does not change the vote count of a particular user for this particular proposal. This also means that there is no need to temporarily transfer user votes to the protocol when votes are casted.

In executeProposal(), if we have more noVotes than yesVotes, it means that proposal should be rejected. When a user calls the executeProposal() function, it will revert due to an underflow error. It would be better to catch this error and inform users who call this function the real revert reason being noVotes > yesVotes.

Governance.sol#L266

    function executeProposal() external {
        uint256 netVotes = yesVotesForProposal[activeProposal.proposalId] -
            noVotesForProposal[activeProposal.proposalId];
        ...      
    }