LMCV part 1 - DAMfinance

An administrator of the dPrime token can burn users' tokens. Admin burning dPrime might break the LMCV contract as there might be less supply than recorded in the LMCV contract.

Code Location

contracts/CollateralJoin.sol

function burn(address from, uint256 value) external {
    uint256 balance = balanceOf[from];
    require(balance >= value, "dPrime/insufficient-balance");

    if (from != msg.sender && admins[msg.sender] != 1) {
        uint256 allowed = allowance[from][msg.sender];
[...]

Hardhat test scenario:

it("HAL-02 admin burns user dPrime", async function () {
    await setupUser(addr1, ["1000", "1000", "1000"]);
    await userLMCV.approveMultiple([lmcvProxy.address, dPrimeJoin.address]);

    // lock some collataral of each token
    await userLMCVProxy.createLoan(collateralBytesList, [fwad("100"), fwad("200"), fwad("300")], fwad("1000"));
    expect(await lmcv.totalNormalizedDebt()).to.equal(fwad("1000"));
    expect(await lmcv.normalizedDebt(addr1.address)).to.equal(fwad("1000"));

    expect(await dPrime.balanceOf(addr1.address)).to.eq(fwad("990"));
    expect(await dPrime.totalSupply()).to.eq(fwad("990"));

    dPrime.burn(addr1.address, fwad("500"));

    expect(await lmcv.totalNormalizedDebt()).to.equal(fwad("1000"));
    expect(await lmcv.normalizedDebt(addr1.address)).to.equal(fwad("1000"));
    expect(await dPrime.balanceOf(addr1.address)).to.be.eq(fwad("490"));
    expect(await dPrime.totalSupply()).to.eq(fwad("490"));
});

PENDING: The DAMfinance team stated that they are planning to implement the recommended fix with the governance module in the future.

There are multiple cases in the LMCV contract when subtracting balances without checks. Such behavior may cause the transaction to fail due to arithmetic errors (integer underflow).

Code Location

The repay function does not perform boundary checks; when the user tries to repay a loan after the interest rate has changed, the transaction may fail with an arithmetic error due to underflow:

Calculating the dPrime amounts and debt:

contracts/LMCV.sol

dPrime[user]            -= normalizedDebtChange * rateMult;
totalDPrime             -= normalizedDebtChange * rateMult;
normalizedDebt[user]    -= normalizedDebtChange;
totalNormalizedDebt     -= normalizedDebtChange;

Calculating new collateral amount:

contracts/LMCV.sol

// Debit locked collateral amount and credit unlocked collateral amount.
uint256 newLockedCollateralAmount   = lockedCollateral[user][collateralList[i]]     -= collateralChange[i];
uint256 newUnlockedCollateralAmount = unlockedCollateral[user][collateralList[i]]   += collateralChange[i];

deflate function calculating protocol deficit:

contracts/LMCV.sol

function deflate(uint256 rad) external {
    address u = msg.sender;
    protocolDeficit[u]      -= rad;
    totalProtocoldeficit    -= rad;
    dPrime[u]               -= rad;
    totalDPrime             -= rad;

    emit Deflate(msg.sender, rad);
}

pullCollateral function updating unlocked collateral:

contracts/LMCV.sol

function pullCollateral(bytes32 collat, address user, uint256 wad) external auth {
    unlockedCollateral[user][collat] -= wad;
    emit PullCollateral(collat, user, wad);
}

moveCollateral function updating unlocked collateral:

contracts/LMCV.sol

function moveCollateral(bytes32 collat, address src, address dst, uint256 wad) external {
    require(approval(src, msg.sender), "LMCV/collateral move not allowed");
    unlockedCollateral[src][collat] -= wad;
    unlockedCollateral[dst][collat] += wad;
    emit MoveCollateral(collat, src, dst, wad);
}

moveDPrime function:

contracts/LMCV.sol

function moveDPrime(address src, address dst, uint256 rad) external {
    require(approval(src, msg.sender), "LMCV/dPrime move not allowed");
    dPrime[src] -= rad;
    dPrime[dst] += rad;
    emit MoveDPrime(src, dst, rad);
}

Below are the Hardhat test cases for underflow issues:

Issues
  ✔ HAL-03 Integer underflow in LMCV pullCollateral function - exit twice (38ms)
  ✔ HAL-03 Integer underflow in LMCV pullCollateral function - exit with more than joined
  ✔ HAL-03 Integer underflow in LMCV moveCollateral - move more than deposited
  ✔ HAL-03 Integer underflow in LMCV moveDPrime (62ms)
  ✔ HAL-03 Integer underflow in LMCV repay - rate updated after loan (98ms)

Source code of test cases:

it("HAL-03 Integer underflow in LMCV pullCollateral function - exit twice", async function () {

    expect(await userLMCV.unlockedCollateral(addr1.address, mockTokenBytes)).to.equal(fwad("100"));

    // exit from CollateralJoin
    let collatJoinConnect = collateralJoin.connect(addr1);
    await collatJoinConnect.exit(addr1.address, fwad("100"));

    expect(await userLMCV.unlockedCollateral(addr1.address, mockTokenBytes)).to.equal(fwad("0"));

    // try to exit for the second time
    await expect(collatJoinConnect.exit(addr1.address, fwad("100")))
        .to.be.revertedWith("Arithmetic operation underflowed or overflowed outside of an unchecked block");
});

it("HAL-03 Integer underflow in LMCV pullCollateral function - exit with more than joined", async function () {

    let collatJoinConnect = collateralJoin.connect(addr1);
    expect(await userLMCV.unlockedCollateral(addr1.address, mockTokenBytes)).to.equal(fwad("100"));

    // exit with more than joined
    await expect(collatJoinConnect.exit(addr1.address, fwad("101")))
        .to.be.revertedWith("Arithmetic operation underflowed or overflowed outside of an unchecked block");

    expect(await userLMCV.unlockedCollateral(addr1.address, mockTokenBytes)).to.equal(fwad("100"));
});

it("HAL-03 Integer underflow in LMCV moveCollateral - move more than deposited", async function () {

    expect(await userLMCV.unlockedCollateral(addr1.address, mockTokenBytes)).to.equal(fwad("100"));
    expect(await userLMCV.unlockedCollateral(addr1.address, mockToken2Bytes)).to.equal(fwad("200"));
    expect(await userLMCV.unlockedCollateral(addr1.address, mockToken3Bytes)).to.equal(fwad("300"));

    await expect(userLMCV.moveCollateral(mockTokenBytes, addr1.address, addr1.address, fwad("200")))
        .to.be.revertedWith("Arithmetic operation underflowed or overflowed outside of an unchecked block");
});

it("HAL-03 Integer underflow in LMCV moveDPrime", async function () {

    await userLMCV.approve(dPrimeJoin.address);
    await userLMCV.loan([mockTokenBytes], [fwad("50")], fwad("1000"), addr1.address);
    let dPrimeJoinConnect = dPrimeJoin.connect(addr1);
    await expect(dPrimeJoinConnect.exit(addr1.address, fwad("1001")))
        .to.be.revertedWith("Arithmetic operation underflowed or overflowed outside of an unchecked block");

    expect(await dPrime.balanceOf(addr1.address)).to.equal(fwad("0"));
    expect(await lmcv.totalDPrime()).to.equal(frad("1000"));
});

it("HAL-03 Integer underflow in LMCV repay - rate updated after loan", async function () {

    // take a loan
    await userLMCV.loan(collateralBytesList, [fwad("50"), fwad("100"), fwad("200")], fwad("2000"), addr1.address);
    expect(await lmcv.dPrime(addr1.address)).to.equal(frad("2000"));
    expect(await userLMCV.normalizedDebt(addr1.address)).to.equal(fwad("2000"));
    expect(await userLMCV.dPrime(addr1.address)).to.equal(frad("2000"));

    // update rate
    await lmcv.updateRate(fray(".1"));

    // repay
    await expect(userLMCV.repay(collateralBytesList, [fwad("50"), fwad("100"), fwad("200")], fwad("2000"), addr1.address))
        .to.be.revertedWith("Arithmetic operation underflowed or overflowed outside of an unchecked block");
});

SOLVED: Added additional require statements to ensure that an underflow does not occur:

LMCV.sol: #275, 282, 294, 409, 420, 530

PSM contract may revert due to an arithmetic error caused by integer underflow when mintFee is set to a large value.

Code Location

contracts/PSM.sol

function createDPrime(address usr, bytes32[] memory collateral, uint256[] memory collatAmount) external {
    require(collateral.length == 1 && collatAmount.length == 1 && collateral[0] == collateralName, "PSM/Incorrect setup");
    uint256 collatAmount18 = collatAmount[0] * to18ConversionFactor; // [wad]
    uint256 fee = _rmul(collatAmount18, mintFee); // rmul(wad, ray) = wad
    uint256 dPrimeAmt = collatAmount18 - fee;

    collateralJoin.join(address(this), collatAmount[0], msg.sender);
[...]

SOLVED: The require statement was added to ensure that mintFee is less than 100%.

Reference: PSM.sol

The deny function is not checking if there are any other active wards before setting wards[usr] = 0. If the user denies himself, when they are the only ward, the contract will lose admin functionality.

Code Location

LMCV module:

contracts/LMCVProxy.sol

function deny(address usr) external auth {
    wards[usr] = 0;
    emit Deny(usr);
}

PSM module:

contracts/PSM.sol

function deny(address usr) external auth {
    wards[usr] = 0;
    emit Deny(usr);
}

dPrime module:

contracts/dPrime.sol

function deny(address usr) external auth {
    admins[usr] = 0;
    emit Deny(usr);
}

CollateralJoin module:

contracts/CollateralJoin.sol

function deny(address usr) external auth {
    wards[usr] = 0;
    emit Deny(usr);
}

CollateralJoinDecimals module:

contracts/CollateralJoinDecimals.sol

function deny(address usr) external auth {
    wards[usr] = 0;
    emit Deny(usr);
}

SOLVED: The ArchAdmin variable was added to the contract. The address assigned to this field cannot be removed from wards/admins mapping via administrate or deny functions, ensuring there is at least one administrator on the contract. To update this address, a new ArchAdmin must be set; then the address can be removed from admins mapping.

Reference:

• LMCV.sol
• LMCVProxy.sol
• PSM.sol
• dPrime.sol
• CollateralJoin.sol
• CollateralJoinDecimals.sol

In case a hack occurs, or an exploit is discovered, the team should be able to pause functionality until the necessary changes are made to the system. To use a THORchain example again, the team behind THOR chain noticed an attack was going to occur well before the system transferred funds to the hacker. However, they were unable to shut the system down fast enough (According to the incident report).

In case of the contracts in scope, only LMCV and LMCVProxy can be stopped/resumed. Other contracts can only be disabled by the cage function (CollateralJoin and CollateralJoinDecimals) or do not have such possibility at all (PSM, dPrimeJoin).

SOLVED: The cage function was modified, and the setLive function was added to the contracts. Now all contracts except dPrime and dPrimeJoin can be stopped/resumed in case of an attack.

• CollateralJoin.sol
• CollateralJoinDecimals.sol
• PSM.sol

The smart contracts, in scope, do not implement granular access control. All the privileged functionality is assigned to one role. This could lead to serious consequences if, for example, a malicious admin decides to take over the platform.

RISK ACCEPTED: The DAMfinance team accepted the risk of this finding and stated that in this role-based admin structure, only smart contracts would have access to specific roles, and a person-controlled owner address would be able to set all of these roles. Since a smart contract can only call functions it has interfaces for and admin access to in this setup, and an owner-level admin hack would have the ability to set itself as any other level admin, this does not seem like a useful check.

The isWithinCreditLimit function does not handle leveraged-only collateral properly. When leveraged-only collateral is passed, and the credit value exceeds the collateral value, the transaction fails with a Division or modulo division by zero error.

Code Location

LMCV module:

contracts/LMCV.sol

function isWithinCreditLimit(address user, uint256 rate) private view returns (bool) {
    bytes32[] storage lockedList = lockedCollateralList[user];
    uint256 creditLimit             = 0; // [rad]
    uint256 leverTokenCreditLimit   = 0; // [rad]
    uint256 noLeverageTotal         = 0; // [wad]
    uint256 leverageTotal           = 0; // [rad]
    for (uint256 i = 0; i < lockedList.length; i++) {
        Collateral memory collateralData = CollateralData[lockedList[i]];

        if(lockedCollateral[user][lockedList[i]] > collateralData.dustLevel){
            uint256 collateralValue = lockedCollateral[user][lockedList[i]] * collateralData.spotPrice; // wad*ray -> rad

            if(!collateralData.leveraged){
                creditLimit += _rmul(collateralValue, collateralData.creditRatio);
                noLeverageTotal += collateralValue / RAY;
            } else {
                leverageTotal += collateralValue;
                leverTokenCreditLimit += _rmul(collateralValue, collateralData.creditRatio);
            }
        }
    }

    // If only leverage tokens exist, just return their credit limit
    // Keep credit ratio low on levered tokens (60% or lower) to incentivize having non levered collateral in the vault
    if(noLeverageTotal == 0 && leverageTotal > 0 && leverTokenCreditLimit >= normalizedDebt[user] * rate){
        return true;
    }

    uint256 leverageMultiple = noLeverageTotal == 0 && leverageTotal == 0 ? RAY : RAY + leverageTotal / noLeverageTotal;

    if (_rmul(creditLimit, leverageMultiple) >= (normalizedDebt[user] * rate)) {
        return true;
    }
    return false;
}

SOLVED: The condition in the isWithinLimit function of the LMCV.sol contract was modified to handle correctly limits for leveraged tokens.

Reference: LMCV.sol

The editCollateralList function takes three arguments: bytes32 collateralName, bool accepted, uint256 position. When adding collateral, only collateraName is used, and position is ignored. When removing collateral, only position is used; the function is not validated if a given collateralName is located in a specified position. Moreover, the function does not check if the given collateral is already added to the list.

Code Location

LMCV module:

contracts/LMCV.sol

function editCollateralList(bytes32 collateralName, bool accepted, uint256 position) external auth {
    if(accepted){
        CollateralList.push(collateralName);
    }else{
        deleteElement(CollateralList, position);
    }
}

SOLVED: The editCollateralList function was removed.

Reference: LMCV.sol

Code Location

Following functions are not validating, that given address is different from zero:

• approve function of dPrime.sol
• constructor of CollateralJoin.sol
• constructor of CollateralJoinDecimals.sol
• constructor of PSM.sol

SOLVED: Zero-address checks were added:

• CollateralJoin.sol
• CollateralJoinDecimals.sol
• PSM.sol

Functions performing important changes on the contract: setLMCV, setDPrimeJoin, setDPrime, and editCollateral are not emitting events to facilitate monitoring of the protocol.

Code Location

LMCVProxy contract, lines: 75, 80, 85, 106

SOLVED: New events were added:

• setLMCV
• setDPrimeJoin
• setDPrime
• editCollateral

The following functions could be declared as external:

• WGLMR.withdraw(uint256)
• WGLMR.totalSupply()
• WGLMR.approve(address,uint256)
• WGLMR.transfer(address,uint256)

SOLVED: Function definitions were updated from public to external:

• WGLMR.withdraw(uint256)
• WGLMR.totalSupply()
• WGLMR.approve(address,uint256)
• WGLMR.transfer(address,uint256)

The following variables could be defined as constant:

• WGLMR.decimals
• WGLMR.name
• WGLMR.symbol

SOLVED: Variable definitions were updated to constant:

• WGLMR.decimals
• WGLMR.name
• WGLMR.symbol

The CollateralList array seems to be used only in setup tests. There is no use of this array in any contract functions.

Code Location

LMCV.sol, line 40

SOLVED: The editCollateralList function and the CollateralList array were removed.

Solidity contracts can use a special form of comments to provide rich documentation for functions, return variables, and more. This special form is named the Ethereum Natural Language Specification Format (NatSpec).

Code Location

LMCV.sol, LMCVProxy.sol, PSM.sol, WGLMR.sol, dPrimeJoin.sol, dPrime.sol, CollateralJoin.sol, CollateralJoinDecimals.sol

PENDING: Natspec documentation is planned for future releases.

It is often more optimal to define parameters as calldata instead of memory for external functions when the parameter is only read.

The following parameters of external functions are stored in memory:

• collateralList, collateralChange parameters of loan, repay and liquidate functions in LMCV contract
• collaterals and amounts in createLoan and repayLoan functions of LMCVProxy contract

After changing those parameters from memory to calldata, gas usage in unit tests was reduced by around 1000.

Code Location

LMCV.sol, 316, 389, 462

LMCVProxy.sol 112, 123

SOLVED: Function definitions were updated with calldata:

• loan
• repay
• seize
• createLoan
• repayLoan