MatricksDeCoder - ..

[sherlock-admin2]

MatricksDeCoder

medium

..

Summary

Critical parameters like addresses and sanity checks of values lack input validation e.g constructor in GovernanceBravoDelegator lacks input validation on all passed-in parameters,

Vulnerability Detail

Take for example the constructor in GovernanceBravoDelegator .sol parameters are not validated as suggested in Compound audit Medium finding, and olympus uses compound governance

Impact

Not checking values allows setting of address to address(0), e.g timelock, ohm address, kernel implementation can be set to address(0).

uint256 votingPeriod_, uint256 votingDelay_, uint256 proposalThreshold_ can be set to arbitrary values that makes the voting module not work or not work as expected. These need to be sanity checked and bounded

Code Snippet

https://github.com/sherlock-audit/2024-01-olympus-on-chain-governance/blob/main/bophades/src/external/governance/GovernorBravoDelegator.sol#L10

onstructor(
        address timelock_,
        address gohm_,
        address kernel_,
        address implementation_,
        uint256 votingPeriod_,
        uint256 votingDelay_,
        uint256 proposalThreshold_
    ) {
        // Admin set to msg.sender for initialization
        admin = msg.sender;

        delegateTo(
            implementation_,
            abi.encodeWithSignature(
                "initialize(address,address,address,uint256,uint256,uint256)",
                timelock_,
                gohm_,
                kernel_,
                votingPeriod_,
                votingDelay_,
                proposalThreshold_
            )
        );

Tool used

1. Manual Analysis
2. OpenZeppelin Audit of Compund https://blog.openzeppelin.com/compound-governor-bravo-audit

Manual Review

Recommendation

Consider bounding inputs to reasonable ranges and excluding certain values, such as address(0) or uint256(0) from being successfully passed in. This will reduce the surface for error when using these functions.

Duplicate of #21