Canto v2 contest - Lambda's results

Lines of code

https://github.com/Plex-Engineer/lending-market-v2/blob/2646a7676b721db8a7754bf5503dcd712eab2f8a/contracts/CNote.sol#L148

Vulnerability details

Impact

In doTransferOut, the underlying balance of the CNote has to be 0 after the transfer. While this works fine when the underlying balance of the CNote was 0 before the call (i.e., in the normal case), the function will always revert when the balance was greater than 0 before the transfer. An attacker can therefore block all withdrawals by sending a tiny amount of the underlying token to the CNote.

Proof of Concept

User A holds multiple CNotes with an arbitrary underlying Token TKN. After some time, he wants to redeem his CNote for TKN and therefore calls redeemFresh. However, before he does that, an attacker transfers 1 TKN to the contract address of the CNote. In doTransferOut, amount is sent to the CNote and then transferred to user A. However, after the transfer, token.balanceOf(address(this)) > 0, meaning the transfer will always fail.

Recommended Mitigation Steps

Store the balance before the transfer and check that the difference is equal to the amount.

Lines of code

https://github.com/Plex-Engineer/lending-market-v2/blob/2646a7676b721db8a7754bf5503dcd712eab2f8a/contracts/NoteInterest.sol#L118 https://github.com/Plex-Engineer/lending-market-v2/blob/2646a7676b721db8a7754bf5503dcd712eab2f8a/contracts/CToken.sol#L209

Vulnerability details

Impact

According to the documentation in InterestRateModel, getBorrowRate has to return the borrow rate per block and the function borrowRatePerBlock in CToken directly returns the value of getBorrowRate. However, the rate per year is returned for NoteInterest. Therefore, using NoteInterest as an interest model will result in completely wrong values.

Recommended Mitigation Steps

Return baseRatePerBlock.

• Existing proposals can be overwritten in the function AddProposal (https://github.com/Plex-Engineer/manifest-v2/blob/3aedf21400a470a50c9be6e38f1ca57f061eca46/contracts/Proposal-Store.sol#L46). This is probably not desired, as an approved proposal that is added to the store should be immutable. Consider checking if the proposal exists in the function.
• This comment in Proposal-Store.sol is wrong and seems to be copied from ERC20DirectBalanceManipulation.sol.
• In NoteInterest.sol, it is mentioned that adjusterCoefficient has a default value of 1, although there is no default value.
• This check in CErc20.sol is problematic for tokens that have multiple entry points (see https://github.com/d-xo/weird-erc20), as an admin can then take over all of the underlying tokens.
• Consider checking for the 0 address in setAccountantContract to avoid errors.
• In ProposalStore.go, this comment refers to the wrong variable.
• In the function doTransferIn of the contract CNote, this comment is wrong, as there is no underflow that can occur there.

In NoteInterest.sol and Timelock.sol, SafeMath is used, although the Solidity version is >= 0.8.0.