Olympus DAO contest - berndartmueller's results

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L181 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L259 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L302-L304

Vulnerability details

Submitted proposals require a certain amount of user endorsements before being activated. Users can endorse as many proposals as they like. The current balance of VOTES tokens of a user is used as a measure of endorsement by a user.

Once a proposal is activated, users can vote with their VOTES token balance. When votes are cast, the current user balance of VOTES tokens used to vote on the proposal is locked in the contract until the proposal is no longer active. 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.

Impact

Once voted on an active proposal, VOTES tokens are locked in the contract. Users cannot endorse other proposals and must wait until they can redeem their votes.

Worst case, all users with VOTES tokens voted on an active proposal and have their tokens locked. Users cannot endorse new proposals. Hence no new proposal can be activated until the currently active proposal is executed or expired.

Proof of Concept

The VOTES balance of a user (VOTES.balanceOf(msg.sender)) is used as a measure of endorsement from a user for a proposal:

policies/Governance.endorseProposal

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

    [...]
}

Voting for a proposal will lock all VOTES token (userVotes) from a user:

policies/Governance.vote

function vote(bool for_) external {
    uint256 userVotes = VOTES.balanceOf(msg.sender);

    if (activeProposal.proposalId == 0) {
        revert NoActiveProposalDetected();
    }

    if (userVotesForProposal[activeProposal.proposalId][msg.sender] > 0) {
        revert UserAlreadyVoted();
    }

    if (for_) {
        yesVotesForProposal[activeProposal.proposalId] += userVotes;
    } else {
        noVotesForProposal[activeProposal.proposalId] += userVotes;
    }

    userVotesForProposal[activeProposal.proposalId][msg.sender] = userVotes;

    VOTES.transferFrom(msg.sender, address(this), userVotes);

    emit WalletVoted(activeProposal.proposalId, msg.sender, for_, userVotes);
}

Votes are locked in the Governance contract once voted for a proposal and can be redeemed later.

policies/Governance.reclaimVotes

function reclaimVotes(uint256 proposalId_) external {
    uint256 userVotes = userVotesForProposal[proposalId_][msg.sender];

    if (userVotes == 0) {
        revert CannotReclaimZeroVotes();
    }

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

    [...]
}

Tools Used

Manual review

Recommended mitigation steps

Consider using, in addition to the freely available votes, the locked votes of a user for endorsing a proposal:

function endorseProposal(uint256 proposalId_) external {
-   uint256 userVotes = VOTES.balanceOf(msg.sender);
+   uint256 userVotes = VOTES.balanceOf(msg.sender) + userVotesForProposal[activeProposal.proposalId][msg.sender];

    if (proposalId_ == 0) {
        revert CannotEndorseNullProposal();
    }

    [...]
}

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/TRSRY.sol#L69

Vulnerability details

The permissioned TRSRY.setApprovalFor function sets approval for specific withdrawer addresses.

However, changing an allowance with this method brings the risk that a withdrawer may use both the old and the new allowance by unfortunate transaction ordering (see the same issue within OZ ERC20)

Impact

A treasury withdrawer can front-run transactions with setApprovalFor(..) and withdraw both the old and the new allowance amount.

Proof of Concept

modules/TRSRY.setApprovalFor

/// @notice Sets approval for specific withdrawer addresses
function setApprovalFor(
    address withdrawer_,
    ERC20 token_,
    uint256 amount_
) external permissioned {
    withdrawApproval[withdrawer_][token_] = amount_;

    emit ApprovedForWithdrawal(withdrawer_, token_, amount_);
}

Tools Used

Manual review

Recommended mitigation steps

Consider implementing increasing and decreasing allowance functions (similar to the OZ ERC20 implementation) to mitigate the race condition.