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

The Invariant can be broken as 1 NOTE does not always equal to 1 cNOTE. #412

Closed
c4-submissions opened this issue Nov 17, 2023 · 7 comments
Closed
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working edited-by-warden insufficient quality report This report is not of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-submissions
Copy link
Contributor

c4-submissions commented Nov 17, 2023

Lines of code

https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/asD/src/asD.sol#L63
https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/asD/src/asD.sol#L52

Vulnerability details

Impact

users will not be able to redeem their asD tokens for equivalent amount of NOTE because when minting cNOTE, 1 cNOTE doesn't always equal 1 NOTE.
image

Link to site here
as of when the image above was taken, you needed exactly 1.0042 NOTES to be able to get 1 cNOTE, and you can see in the mint() function,

    function mint(uint256 _amount) external {
        CErc20Interface cNoteToken = CErc20Interface(cNote);
        IERC20 note = IERC20(cNoteToken.underlying());
        SafeERC20.safeTransferFrom(note, msg.sender, address(this), _amount);
        SafeERC20.safeApprove(note, cNote, _amount);
        uint256 returnCode = cNoteToken.mint(_amount);
        // Mint returns 0 on success: https://docs.compound.finance/v2/ctokens/#mint
        require(returnCode == 0, "Error when minting");
        _mint(msg.sender, _amount);
    }

if a user deposits 1 NOTE, the contract gets 0.9958 cNOTE and mint to the user 1 asD, whenever the user wants to burn that 1 asD for an equivalent 1 NOTE, the call to cNoteToken.redeemUnderlying(_amount) in burn() will revert as the contract's balance of cNOTE will not be able to redeem 1 NOTE from the cNOTE contract.

The issue with this is that

  1. This breaks the invariant of the protocol.
    Click here

Main invariants
asD: It should always be possible to redeem 1 asD for 1 NOTE.

  1. Some users can unknowingly take other users share of NOTE.

check the Proof of Concept

Proof of Concept

  • User A and User B mint 1 asD each when NOTE != cNOTE where 1.0042 NOTE = 1 cNOTE.
  • The contract gets 1.99 cNOTEs.
  • User A burns his 1 asD to get back his 1 NOTE and the transaction is successful as the contract's balance of cNOTE is 1.99.
  • User B wants to burn his 1 asD for his 1 NOTE back but the transaction fails as 0nly 0.99 cNOTEs are in the contract.

Here you can see User A has unknowingly taken a share of User B's N0TE causing loss to User B, NOTE This is a simplified Instance, where in huge amount a lot of issues can arise as many users are in the protocol.

Tools Used

Manual review

Recommended Mitigation Steps

my recommendation would be to require that the amount of cNOTE minted for the contract is equivalent to the amount of asD token to be minted to the user on the call to mint().

Assessed type

Other

@c4-submissions c4-submissions added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Nov 17, 2023
c4-submissions added a commit that referenced this issue Nov 17, 2023
@code4rena-admin code4rena-admin changed the title The Invariant can be broken as 1 NOTE != 1 cNOTE. The Invariant can be broken as 1 NOTE does not always equal to 1 cNOTE. Nov 17, 2023
@c4-pre-sort
Copy link

minhquanym marked the issue as insufficient quality report

@c4-pre-sort c4-pre-sort added the insufficient quality report This report is not of sufficient quality label Nov 18, 2023
@minhquanym
Copy link
Member

Seems invalid. _amount is underlying amount, not cNote amount

@c4-judge
Copy link

MarioPoneder marked the issue as unsatisfactory:
Invalid

@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Nov 29, 2023
@shealtielanz
Copy link

shealtielanz commented Nov 30, 2023

hi @MarioPoneder
I believe there is a misunderstanding of the issue I submitted and it has been totally disregarded and misjudged.
In this reports I stated the following

  • The Invariant is broken as 1 asd doesn't always equal to 1 Note, because the when depositing 1 note to the CLM, the contract doesn't always get 1 cNOTE back.(1 NOTE != 1 cNOTE)
  • Not only does this break the invariant, it can lead to a loss of funds to different users in the protocol.

This issue is valid and a High, I request you review this issue again.

Elaboration
As shown in the Image above, as of then you would require about 1.0042 NOTEs to get 1 cNOTE, when the mint function deposits 1 NOTE to the CLM to get equivalent cNOTE uint256 returnCode = cNoteToken.mint(_amount) due to the price difference, the contract would not get back 1 cNOTE but a less amount but it mints for the user 1 asD in return _mint(msg.sender, _amount);.
Assuming that was the first time mint, if the user wants to claim back their 1 NOTE with their 1 asD after a period of time, it would not be possible as the CLM requires 1 cNOTE to return 1 NOTE, and the cNOTE in the contract is not up to the required amount. this issue is similar to #451 but that talks about rounding issues on the return, but mine talks about the price difference, although our mitigation is the same.
IMG_6355
You can see the price of NOTE and cNOTE are not exactly linear as more NOTEs will be required for 1 cNOTE
This breaks the invariant of the protocol and can cause issues as described in the main report.

Quick example

  • The Contract is newly created
  • User calls mint() with 1 NOTE to get 1 asD
  • mint() deposits the 1 NOTE to the CLM and mints for the user 1 asD.
  • However, CLM doesn't give the contract 1 cNOTE for that 1 NOTE it recieved from the contract, but a less amount which is 0.996 approximately
  • After 2 days the User wants back their 1 NOTE back, on the call to burn, contract cannot recieve the 1 NOTE back from CLM as the CLM requires 1 cNOTE to get 1 NOTE but the contract only has 0.996 cNOTE.

You can see that the 1 asD to 1 NOTE peg is broken.

Thank you for your time sir, I appreciate it.

@MarioPoneder
Copy link

Continuing your example:
Why do you think it requires one 1 cNOTE to redeem 1 NOTE?
The redeem method uses the same exchange rate as the mint method.

This exchange rate is ever increasing, see https://docs.compound.finance/v2/ctokens/#exchange-rate

Each cToken is convertible into an ever increasing quantity of the underlying asset, as interest accrues in the market.

Therefore you can always get back the amount of NOTE tokens you deposited.

@shealtielanz
Copy link

Thanks @MarioPoneder
for clearing my misunderstanding, I didn't think to put that into consideration.

@MarioPoneder
Copy link

Thank you!
We are all making mistakes, this is part of the process.
All the best and see you in future contests!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working edited-by-warden insufficient quality report This report is not of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

7 participants