Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect accounting for yieldBoxShares in SGLLiquidation results in wrongly read values #1349

Open
code423n4 opened this issue Aug 4, 2023 · 5 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) M-17 primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/singularity/SGLLiquidation.sol#L29-L65

Vulnerability details

Impact

Wrong accounting is done for yieldBoxShares while liquidating through the SGLLiquidation#liquidate function. This results in wrong values being read from the yieldBoxShares() function in the Singularity market. This might result in unintended behavior from other contracts which read from this function.

Proof of Concept

When a user adds collateral to the singularity market through SGLCollateral#addCollateral, the SGLLendingCommon#_addTokens function is called as we can see in the following code lines:

  1. https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/singularity/SGLLendingCommon.sol#L16-L38

  2. https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/singularity/SGLCommon.sol#L174-L194

As we can see here, yieldBoxShares is updated to include the number of shares supplied while supplying collateral. Therefore, if the user borrows an amount, and is unable to pay and hence is to be liquidated, the SGLLiquidation#liquidate function is called on them. This should update the user’s yieldBoxShares to a new value, but they do not do this. Both the _closedLiquidation and _orderBookLiquidation methods do not update yieldBoxShares.

This results in wrong values stored as yieldBoxShares owned by a particular user, and hence when another contract calls Singularity#yieldBoxShares, the wrong value is supplied.

Tools Used

Manual Review

Recommended Mitigation Steps

Update yieldBoxShares when liquidating.

Assessed type

Other

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Aug 4, 2023
code423n4 added a commit that referenced this issue Aug 4, 2023
@c4-pre-sort
Copy link

minhquanym marked the issue as primary issue

@c4-pre-sort c4-pre-sort added the primary issue Highest quality submission among a set of duplicates label Aug 9, 2023
@c4-sponsor
Copy link

0xRektora marked the issue as disagree with severity

@c4-sponsor c4-sponsor added the disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) label Aug 25, 2023
@0xRektora
Copy link

Informational.

@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Aug 25, 2023
@c4-sponsor
Copy link

0xRektora marked the issue as sponsor confirmed

@c4-judge
Copy link

dmvt marked the issue as selected for report

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Sep 30, 2023
@0xRobocop 0xRobocop mentioned this issue Oct 5, 2023
@C4-Staff C4-Staff added the M-17 label Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) M-17 primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

6 participants