code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/peg/NonCustodialPSM.sol#L259 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/pcv/compound/ERC20CompoundPCVDeposit.sol#L28

Vulnerability details

Impact

The documents specified that NonCustodialPSM will directly send collateral into yield venues for system surplus. However, implementation of NonCustodialPSM.mint only sends collateral to PCVDeposit and does not call the pcvDeposit.deposit function. Collateral within the PCVDeposit contract will not gain any revenue, and can not be withdrawn until sent into yield venue by an independent call to pcvDeposit.deposit. It is unreasonable to expect users of volt protocol to know this, and if a considerable amount of collateral stays in PCVDeposit for a long enough time frame, volt might become under-collateralized, thus undermining the entire protocol.

Proof of Concept

FEI PCVDeposit serves as a tunnel to transfer collateral from NonCustodialPSM to Fuse Pool. The current implementation can be broken into two parts

Sender transfers collateral into PCVDeposit
Sender calls pcvDeposit.deposit to move collateral within PCVDeposit into Fuse Pool

    function deposit() external override whenNotPaused {
        uint256 amount = token.balanceOf(address(this));

        token.approve(address(cToken), amount);

        // Compound returns non-zero when there is an error
        require(
            CErc20(address(cToken)).mint(amount) == 0,
            "ERC20CompoundPCVDeposit: deposit error"
        );

        emit Deposit(msg.sender, amount);
    }

Noticebly, if pcvDeposit.deposit is not called, the collateral stays within PCVDeposit, and will not be sent into any yield venue for surplus.

However, in the NonCustodialPSM.mint the function only completes part 1 of a complete deposit (transfer collateral to PCVDeposit), and does not handle the second part pcvDeposit.deposit.

    function mint(...) ... {
        ...
        underlyingToken.safeTransferFrom(
            msg.sender,
            address(pcvDeposit),
            amountIn
        );

        //missing deposit here

        ...
    }

This creates a risk where the collateral will be held by PCVDeposit instead of sent to Fuse Pool if user does not realize calling NonCustodialPSM.mint is necessary. The collateral then stays in PCVDeposit until some other happens to call pcvDeposit.deposit, and does the second transfer in place of original minter.

We can't appropriately decide whether this mechanism is by design or accidental, since in the unit test contract of NonCustodialPSM, out of the several mint called, only one has a pairing deposit. Eventually, we judged that the current mechanism will be confusing to end users, and might cause further problems regarding volt value (if yield generated < inflation, volt will not be fully collateralized, and cannot be redeemed when desired).

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Add a call to the pcvDeposit.deposit function in NonCustodialPSM.mint.

ElliotFriedman commented 2 years ago

This is intended because calling deposit on the PCVDeposit is very expensive in terms of gas. Offchain workers will be set up to call the mint function once every 24 hours or once there is a large enough deposit build up.

jack-the-pug commented 2 years ago

Downgrading to QA as the issues does not exist in practice, and the impact is minor.

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