[M-05] Moving collateral between vaults, when the former one is undercollateralized

[obheda12]

Summary

In the VRCauldron.sol smart contract, the spot oracle's ratio is not bound by a lower and upper limit, which could potentially lead to incorrect collateral transfers between vaults when one is undercollateralized.

The setSpotOracle() function in VRCauldron.sol lacks validation for the ratio value, allowing it to be set to any value as shown below:

File: VRCauldron.sol

function setSpotOracle(
        bytes6 baseId,
        bytes6 ilkId,
        IOracle oracle,
        uint32 ratio
) external auth {
    require(assets[baseId] != address(0), "Base not found");
    require(assets[ilkId] != address(0), "Ilk not found");
    spotOracles[baseId][ilkId] = DataTypes.SpotOracle({
        oracle: oracle,
        ratio: ratio // With 6 decimals. 1000000 == 100% //
    }); // Allows to replace an existing oracle.
    emit SpotOracleAdded(baseId, ilkId, address(oracle), ratio);
}

For correct operation, the minimal ratio value should be 1000000 (100%), because the VRCauldron::_level() function normalizes the ratio to 18 decimals:

File: VRCauldron.sol

function _level(
        VRDataTypes.Vault memory vault_,
        DataTypes.Balances memory balances_
) internal returns (int256) {
    ...
    uint256 ratio = uint256(spotOracle_.ratio) * 1e12;
    ...
}

Assuming the spot oracle's ratio is accidentally set to 1e4, and there is an undercollateralized vault, calling the VRCauldron::stir() function to move collateral between vaults would result in unexpected behavior. The stir() function checks if the vault is undercollateralized at origin when moving collateral:

File: VRCauldron.sol

function stir(
        bytes12 from,
        bytes12 to,
        uint128 ink,
        uint128 art
)
    external
    auth
    returns (DataTypes.Balances memory, DataTypes.Balances memory)
{
    ...
    if (ink > 0)
        require(
            _level(vaultFrom, balancesFrom) >= 0,
            "Undercollateralized at origin"
        );
    ...
}

Due to the incorrect ratio value, the VRCauldron::_level() function should return a value less than 0, but it will return a positive value instead.

The ratio

PoC

This ration only demonstrates the broken logic when moving collateral, however, it is possible that with different ratio values other logic can also be subverted. Add the test case in VRCauldron.t.sol file:

function testStirUndercollateralizedAtOrigin() public {
    console.log("can stir from vault with undercollateralized origin");
    // Set spotOracle ratio as 1e4
    cauldron.setSpotOracle(baseId, usdcId, spotOracle, 1e4);

    cauldron.pour(vaultId, 1e17, ART.i128());
    cauldron.build(address(this), otherVaultId, baseId, usdcId);

    // Vault is undercollateralized
    // Stir should revert, but doesn't, because the SpotOracle ratio is wrong
    cauldron.stir(vaultId, otherVaultId, INK.u128(), 0);
}

Test result

Running 1 test for test/VRCauldron.t.sol:CauldronStirTests
[PASS] testStirUndercollateralizedAtOrigin() (gas: 183717)
Test result: ok. 1 passed; 0 failed; finished in 10.76ms

Recommendations

To prevent inconsistencies in the _level() function, add a validation check for the ratio value in the setSpotOracle() function, ensuring it has 6 decimals and is within an acceptable range:

function setSpotOracle(
    bytes6 baseId,
    bytes6 ilkId,
    IOracle oracle,
    uint32 ratio
) external auth {
    require(assets[baseId] != address(0), "Base not found");
    require(assets[ilkId] != address(0), "Ilk not found");
+   require(ratio >= 1000000 && ratio <= 9999999, "Wrong ratio value");
    spotOracles[baseId][ilkId] = DataTypes.SpotOracle({
        oracle: oracle,
        ratio: ratio // With 6 decimals. 1000000 == 100% //
    }); // Allows to replace an existing oracle.
    emit SpotOracleAdded(baseId, ilkId, address(oracle), ratio);
}

Adding this validation will help ensure that the ratio values are within the appropriate range, preventing unexpected behavior when moving collateral between vaults.

[iamsahu]

Sames as #3