implementation contract for proxy pattern in MyStrategy and Vault are uninitialized and can be initialized by attacker and cause damage

[code423n4]

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L51-L76

Vulnerability details

Impact

There is no constructor() to set state of implementation contract to initialized in the proxy pattern so it's possible for attacker to call initialize for implementation contract and set its values and take control of it. if there are some logics in contract like self-destruct, attacker can perform DOS. so it's safer to initialize implementation contract too by constructor()

Proof of Concept

This is initialize() code in MyStrategy:

    /// @dev Initialize the Strategy with security settings as well as tokens
    /// @notice Proxies will set any non constant variable you declare as default value
    /// @dev add any extra changeable variable at end of initializer as shown
    function initialize(address _vault) public initializer {
        assert(IVault(_vault).token() == address(AURA));

        __BaseStrategy_init(_vault);

        want = address(AURA);

        /// @dev do one off approvals here
        // Permissions for Locker
        AURA.safeApprove(address(LOCKER), type(uint256).max);

        AURABAL.safeApprove(address(BALANCER_VAULT), type(uint256).max);
        WETH.safeApprove(address(BALANCER_VAULT), type(uint256).max);

        // Set Safe Defaults
        withdrawalSafetyCheck = true;

        // Process locks on reinvest is best left false as gov can figure out if they need to save that gas
    }

As you can see it sets the value of vault by calling __BaseStrategy_init() and defines some admin access to contract.

    function governance() public view returns (address) {
        return IVault(vault).governance();
    }

by calling initialize() and setting vault attacker can access all the functions in contract. if the proxy contracts uses some state of implementation contract if there is selfdestruct logic in contract attacker can uses them and harm the protocol. There is no constructor in MyStrategy to set the state of contract to initialized for implementation contract.

Tools Used

VIM

Recommended Mitigation Steps

add a constructor and set the state of implementation contract to initialized

[GalloDaSballo]

Disagree because we don't have selfdestruct nor a way to change implementation

In lack of an actual POC I must fully disagree

[jack-the-pug]

if there are some logics in contract like self-destruct, attacker can perform DOS.

No delegateCall, no way to selfdestruct.