search

code-423n4 / 2021-12-yetifinance-findings

0 stars 0 forks source link

_openTroveInternal() within openTrove function on line 275 (BorrowOperations.sol) has an error in accounting logic that will affect important calculations such as vars.YusdFee et al #295

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

0xwags

Vulnerability details

Impact

_getTotalVariableDepositFee () will return the wrong amount due to a wrong input and incorrect ordering of arguments on line 280.VC_out is set to 0 and MaxFeePercentageFactor is overwritten by VC_out(line 683-684).

Proof of Concept

Calculations of vars.yUsdfee is incorrect. Furthermore, this is used to determine vars.netDebt on line 284. Calculation using compositeDebt on lines 289,292 & 297 will also be affected.

Line 696( value remains unchanged in vars.activePoolVCPost, since the VcOut is set to zero, any VC that is in the active pool will be stuck that needs to be removed and therefore will not reflect the correct amount if there's any non-zero value(VC_out) in the activePool ).

Affects whitelistfee(line 708), since incorrect calculation(in external call to whiteList.getFeeAndUpdate()) is included on line 714 and value will still be the same as vars.activePoolTotalVC on line 713..

Affects calculation at line 722 since the incorrect value of VC_out would be included in the ‘whitelistFee’ on lines 719-720.

Incorrect fee calculation passed to user on line 724(_requireUserAcceptsFee()) .Consequently, since the VC_out overwrites the MaxFeePercentageFactor, this parameter will be used in the internal call to LiquityBase.sol to derive the ‘feePercentage’ on line 140.

Also, in the _triggerDepositFee() on line 725, fees will be affected that will be sent to the sYeti address.

Tools Used

Manual Analysis

Recommended Mitigation Steps

In BorrowOperations.sol line 280, the second vars.VC that represents the VC_out on line 684 should not be 0 and correct the ordering of the argument such that the _maxFeePercentageFactor(line 685) is not overwritten on line 280.

kingyetifinance commented 2 years ago

@LilYeti: Not sure what the issue here is. VC_out represents the amount of collateral that the user is removing in this tx. In open trove, they are not removing any collateral, so the tx will essentially on line 696, correctly calculate that the VC post tx will just be adding the VC_in. However, if the warden meant something else, would like to know that.

alcueca commented 2 years ago

I can't understand the issue either. Linking of actual lines referenced might have helped. Striking it as invalid.