sherlock-audit / 2024-04-interest-rate-model-judging

2 stars 1 forks source link

sakshamguruji - Double Accounting Of EXA Reserves While Canceling a Vest #219

Closed sherlock-admin4 closed 2 months ago

sherlock-admin4 commented 2 months ago

sakshamguruji

high

Double Accounting Of EXA Reserves While Canceling a Vest

Summary

When a user cancels his vest ,the EXA he put up as reserves is returned back , but this is transfer of EXA reserves is accounted twice and thus user receives twice the reserves he put up.

Vulnerability Detail

Let's break down the cancel function ->

1.) A user wants to cancel his vest , calls cancel() with the streamIds for his vests.

https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/periphery/EscrowedEXA.sol#L133

2.) It is made sure the recipient of the vest is the msg.sender

3.) streamReserves are calculated adding up all the reserves for the streamIds (summing up all the reserve amounts for the user's vests).

4.) Then the sablier's cancel function is called , that function invokes a callback function i.e. onStreamCanceled()

5.) That function https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/periphery/EscrowedEXA.sol#L188-L192

calls returnReserves() at L192 and that function ->

function returnReserve(uint256 streamId, address recipient) internal {
    uint256 reserve = reserves[streamId];
    delete reserves[streamId];
    exa.safeTransfer(recipient, reserve);
  }

We can see that the streamId's reserve is returned/transferred here.

6.) Therefore in the for loop at L135 , all the reserves associated with the streamIds would be returned to the recipient.

7.) But , at L147 here https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/periphery/EscrowedEXA.sol#L147 we again transfer the streamReserves (summation of all reserves) to the recipients , leading to recipient receiving twice the reserves he should have.

Impact

The recipient would always receive twice the number of reserves he should have received due to double accounting.

Code Snippet

https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/periphery/EscrowedEXA.sol#L147

https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/periphery/EscrowedEXA.sol#L143

Tool used

Manual Review

Recommendation

Don't perform the transfer of reserves at L147

santipu03 commented 2 months ago

This issue is invalid because the callback onStreamCanceled will only be called if the stream was canceled directly through Sablier and not via EscrowedEXA::cancel.