code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/core/Core.sol#L27 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/refs/CoreRef.sol#L22 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/refs/CoreRef.sol#L199

Vulnerability details

Impact

vcon address is allowed to be updated by GOVERNOR in Core, however, this change will not be reflected in CoreRef._vcon. Moreover, since CoreRef._vcon cannot be updated due to contract design, it is also impossible to fix this manually. We are not yet sure how vcon will be used throughout the volt protocol, since details have not yet been made clear and code does not include related implementations. Consequently, it is impossible to estimate the exact impact. However, this desync between contracts seem dangerous enough to raise our attention, hence this report to inform the volt team about it.

Proof of Concept

In Core, vcon is allowed to be updated by GOVERNORs

    function setVcon(IERC20 _vcon) external onlyGovernor {
        vcon = _vcon;

        emit VconUpdate(_vcon);
    }

But in CoreRef, a contract inherited by several other ones including NonCustodialPSM, GlobalRateLimitedMinter, ERC20CompountPCVDeposit and Volt, _vcon is fixed upon initialization and cannot be further updated

    IERC20 private immutable _vcon;
    ...
    constructor(address coreAddress) {
        ...
        _vcon = ICore(coreAddress).vcon();
        ...
    }

Thus if GOVERNORS ever updated vcon in Core, the state between Core and all other Volt protocol components will mismatch.

Currently _vcon is not used in any place within the Volt protocol, but judging from the description in whitepapaer, future governance will be based on it, thus any potential desync will be devastating.

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

There are several possible solutions.

The first is to dynamically fetch vcon from the Core whenever CoreRef uses it, and avoid storing a static copy locally.

    function vcon() public view override returns (IERC20) {
        return _volt.vcon();
    }

The second is to expose a public API to update _vcon in CoreRef, however, this approach might not be especially favorable since many components will require updates at once, and it is highly possible that future GOVERNORs miss some of them while doing updates.

ElliotFriedman commented 2 years ago

Agreed this is an issue, however if the VCON address is updated, contracts that need to reference the new value will need to be redeployed to cache this new address when CoreRef is instantiated.

jack-the-pug commented 2 years ago

Based on the severity of the impact, I'm downgrading this to Medium.

code-423n4 / 2022-03-volt-findings