Collateral parameters can be overwritten

[code423n4]

Handle

cmichel

Vulnerability details

It's possible to repeatedly add the first collateral token in validCollateral through the Whitelist.addCollateral function. The validCollateral[0] != _collateral check will return false and skip further checks.

POC

Owner calls addCollateral(collateral=validCollateral[0]):

function addCollateral(
    address _collateral,
    uint256 _minRatio,
    address _oracle,
    uint256 _decimals,
    address _priceCurve, 
    bool _isWrapped
) external onlyOwner {
    checkContract(_collateral);
    checkContract(_oracle);
    checkContract(_priceCurve);
    // If collateral list is not 0, and if the 0th index is not equal to this collateral,
    // then if index is 0 that means it is not set yet.
    // @audit evaluates validCollateral[0] != validCollateral[0] which is obv. false => skips require check
    if (validCollateral.length != 0 && validCollateral[0] != _collateral) {
        require(collateralParams[_collateral].index == 0, "collateral already exists");
    }

    validCollateral.push(_collateral);
    // overwrites parameters
    collateralParams[_collateral] = CollateralParams(
        _minRatio,
        _oracle,
        _decimals,
        true,
        _priceCurve,
        validCollateral.length - 1, 
        _isWrapped
    );
}

Impact

The collateral parameters collateralParams are re-initialized which can break the existing accounting. The collateral token also exists multiple times in validCollateral.

Recommended Mitigation Steps

Fix the check. It should be something like:

if (validCollateral.length > 0) {
    require(collateralParams[_collateral].index == 0 && validCollateral[0] != _collateral, "collateral already exists");
}

[kingyetifinance]

Duplicate with #142

[alcueca]

Taking as main