code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/DBR.sol#L284-L292 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/DBR.sol#L313-L317

Vulnerability details

Impact

Repayment/burning of due DBR tokens is currently not enforced, which enables a user to simply switch addresses to extend their borrowing duration, which eventually leads to almost 0% interest paid if performed repeatedly, breaking one of the core intentions of having DBR in the first place (representing borrowing rights that gradually fade over a year).

Proof of Concept

Assume a user borrows an amount x worth of DOLA by providing enough collateral and also buying x amount of DBR. Lets say the exchange rate is 1 DBR = 0.01 DOLA, which would be effectivey 1% interest per year paid upfront. Now that user can hold his DOLA for 1 year, before he would get into a deficit. Briefly before getting into said deficit he could just repay his debt and move collateral+dbr tokens to a new address and repeat the process for another year, effectively halving the interest paid.

The repayment of the debt happens in Market.repay, which reduces the debt by transferring DOLA from the user back to the market:

function repay(address user, uint amount) public {
    uint debt = debts[user];
    require(debt >= amount, "Insufficient debt");
    debts[user] -= amount;
    totalDebt -= amount;
    dbr.onRepay(user, amount);
    dola.transferFrom(msg.sender, address(this), amount);
    emit Repay(user, msg.sender, amount);
    }

The call to dbr.onRepay only serves to perform accounting for a users global debt and due DBR tokens:

function onRepay(address user, uint repaidDebt) public {
    require(markets[msg.sender], "Only markets can call onRepay");
    accrueDueTokens(user);
    debts[user] -= repaidDebt;
}

function accrueDueTokens(address user) public {
    uint debt = debts[user];
    if(lastUpdated[user] == block.timestamp) return;
    uint accrued = (block.timestamp - lastUpdated[user]) * debt / 365 days;
    dueTokensAccrued[user] += accrued;
    totalDueTokensAccrued += accrued;
    lastUpdated[user] = block.timestamp;
    emit Transfer(user, address(0), accrued);
}

The Market.withdraw function does not interact with the DBR contract at all, so the user can perform both repayment and withdrawal without being impacted by the amount of his accrued DBR tokens. He can then simply switch addresses to leave this accrued DBR debt behind and reuse his DBR tokens on a new address for a fresh borrow with the full duration of a year, which would not be possible on his old address:

function onBorrow(address user, uint additionalDebt) public {
    ...
    //this would not be 0 on the old address if borrowed for a little while longer
    require(deficitOf(user) == 0, 
    ...
}

Tools Used

Manual Review

Recommended Mitigation Steps

Add an internal burning function to DBR that reduces dueTokensAccrued of a user and totalDueTokensAccrued. This function should be called before performing a withdrawal of collateral and revert, if the user does not have DBR tokens equal to dueTokensAccrued[user]

The README states As time progresses, DBR will be burnt from the borrower's wallet at a rate that depends on their debt. According to the sponsor information on Discord this is what the accrueDueTokens function (see above) is supposed to do. Currently, this function only tracks the due tokens but does not burn them. So an alternative to implementing a burn on withdrawal would be to enforce burns in this function.

If the term "burn" is only meant to be figurative and should be reflected in the accrual of due tokens as it is implemented right now, a third option could be to simply restrict transfers of tokens that would leave the sender with a deficit. This would effectively resemble a burn as the accrued DBR debt can not be reduced in the current implementation.

d3e4 commented 2 years ago

reuse his DBR tokens on a new address

Transfer of DBR- tokens is done by DBR.transfer() or DBR.transferFrom(). These require(balanceOf(from) >= amount, "Insufficient balance");. DBR.balanceOf() looks like this:

function balanceOf(address user) public view returns (uint) {
    uint debt = debts[user];
    uint accrued = (block.timestamp - lastUpdated[user]) * debt / 365 days;
    if(dueTokensAccrued[user] + accrued > balances[user]) return 0;
    return balances[user] - dueTokensAccrued[user] - accrued;
}

balances[user] - dueTokensAccrued[user] - accrued is effectively only the remaining tokens after due tokens have been "burnt".

The accounting of DBR is done a little differently than perhaps expected. It is essentially a construction of a signed integer, similar to the formal construction of integers in mathematics as an ordered pair of a positive part and a "negative" part.

Minh-Trng commented 2 years ago

reuse his DBR tokens on a new address

Transfer of DBR- tokens is done by DBR.transfer() or DBR.transferFrom(). These require(balanceOf(from) >= amount, "Insufficient balance");. DBR.balanceOf() looks like this:
function balanceOf(address user) public view returns (uint) {
    uint debt = debts[user];
    uint accrued = (block.timestamp - lastUpdated[user]) * debt / 365 days;
    if(dueTokensAccrued[user] + accrued > balances[user]) return 0;
    return balances[user] - dueTokensAccrued[user] - accrued;
}
balances[user] - dueTokensAccrued[user] - accrued is effectively only the remaining tokens after due tokens have been "burnt".

The accounting of DBR is done a little differently than perhaps expected. It is essentially a construction of a signed integer, similar to the formal construction of integers in mathematics as an ordered pair of a positive part and a "negative" part.

good catch, missed that

c4-judge commented 2 years ago

0xean marked the issue as unsatisfactory: Invalid

code-423n4 / 2022-10-inverse-findings