sherlock-audit / 2024-04-teller-finance-judging

13 stars 11 forks source link

0x73696d616f - Issue #497 'Add parameter to lender accept bid for MaxMarketFee' from previous audit is still present #125

Open sherlock-admin2 opened 7 months ago

sherlock-admin2 commented 7 months ago

0x73696d616f

medium

Issue #497 'Add parameter to lender accept bid for MaxMarketFee' from previous audit is still present

Summary

Issue #497 from the previous Sherlock audit was not fixed in the current code and is still present.

Vulnerability Detail

The vulnerability is well explain in the mentionedl link above, essentially any market owner may change the marketplace fee while frontrunning a borrower and getting more funds in return.

A PR with the fix was mentioned in the comments but it was never merged.

From the docs, the issue is valid as long as there is not a won't fix label.

Impact

Borrower pays more marketplace fees than expected due to malicious market owner.

Code Snippet

TellerV2::lenderAcceptBid()

function lenderAcceptBid(uint256 _bidId)
    ...
{
    ...
    amountToMarketplace = bid.loanDetails.principal.percent(
        marketRegistry.getMarketplaceFee(bid.marketplaceId)
    ...
}

Tool used

Manual Review

Vscode

Recommendation

The recommendation from issue #497 are good:

Add a timelock delay for setMarketFeePercent/setProtocolFee allow lenders to specify the exact fees they were expecting as a parameter to lenderAcceptBid

nevillehuang commented 6 months ago

Attaching LSW comments for consideration:

invalid, it is fixed within the market contract, where there's a max fee param that markets can set.

Can't seem to find logic relating to above and don't see it as per here

sherlock-admin2 commented 6 months ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/teller-protocol/teller-protocol-v2-audit-2024/pull/38/files

sherlock-admin2 commented 5 months ago

The Lead Senior Watson signed off on the fix.