code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/main/VaultTracker/LibCompound.sol#L26

Vulnerability details

Impact

Some coumpound tokens will work with the current LibCompound, but some won't because they have a slightly different implementation. The compound USDC (cUSDC) is one of these.

Proof of Concept

Taking the cUSDC token: https://etherscan.io/address/0x39aa39c021dfbae8fac545936693ac917d5e7563#code

function borrowRatePerBlock() external view returns (uint) {
     (uint opaqueErr, uint borrowRateMantissa) = interestRateModel.getBorrowRate(getCashPrior(), totalBorrows, totalReserves);
     require(opaqueErr == 0, "borrowRatePerBlock: interestRateModel.borrowRate failed"); // semi-opaque
     return borrowRateMantissa;
 }

As you can see, getBorrowRate() here returns a tuple (uint256, uint256), where a single value representing the borrowRateMantissa is expected. The first value is used to know if the borrowRate is valid, and the other is the borrow rate. This library is then not compliant with all Compound tokens.

Tools Used

pycharm

Recommended Mitigation Steps

Create a LibCompound2 that store a struct from the return values of the getBorrowRate and check if opaqueErr != 0 to return the borrowRateMantissa, and make sure to use this alternative library for some Compound tokens when needed.

JTraversa commented 2 years ago

Honestly, severity is probably accurate but scope was restricted away from the external Libs?

There is no way we would've noticed the issue, and will attempt to implement remediation, however the repo itself lives here

JTraversa commented 2 years ago

This would have been a prohibitively difficult issue to identify and may deserve proper reward regardless of scope. I've also reported it to the T11s repo.

bghughes commented 2 years ago

Alrighty, thank you @JTraversa for sharing your reasoning. I agree that this is an edge case that should be handled and cUSDC is a popular money market. Moreover, it should be included in the scope IMO because you all care about the integrations you are adding in v3 and agree yourself that this is valid.

Ensuring the new Compounding.sol library properly calculates the exchangeRate for each external protocol.

Ensuring the new withdraw and deposit methods on Swivel.sol properly encapsulate external protocol interactions.

bghughes commented 2 years ago

Also adding Help Wanted temporarily given the potential scope issue. Will revisit.

robrobbins commented 2 years ago

coming back to this later...

0xean commented 2 years ago

Would love it if the Sponsors would tip the warden for the good find, but to keep the contest fair to all wardens, this file was out of scope per the readme.


So the only things explicitly NOT in scope:
- Details of 5095 implementation (the EIP isnt final yet)
- A few external Libs: 
        - FixedPointMathLib
        - LibCompound
        - LibFuse
        - Safe.sol
- Some older stuff not worth a review:
        - Hash.sol
        - Sig.sol

Closing as invalid

code-423n4 / 2022-07-swivel-findings