Canto contest - Tutturu's results

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/CNote.sol#L43 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/CNote.sol#L114 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/CNote.sol#L198 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/CNote.sol#L310

Vulnerability details

Impact

The contract expects the balance of the underlying token to == 0 at all points when calling the contract functions by requiring getCashPrior() == 0, which checks token.balanceOf(address(this)) where token is the underlying asset.

An attacker can transfer any amount of the underlying asset directly to the contract and make all of the functions requiring getCashPrior() == 0 to revert.

Proof of Concept

CNote.sol#L43 CNote.sol#L114 CNote.sol#198 CNote.sol#310

1. Attacker gets any balance of Note (amount = 1 token)
2. Attacker transfers the token to CNote which uses Note as an underlying asset, by calling note.transfer(CNoteAddress, amount). The function is available since Note inherits from ERC20
3. Any calls to CNote functions now revert due to getCashPrior() not being equal to 0

Recommended Mitigation Steps

Instead of checking the underlying token balance via balanceOf(address(this)) the contract could hold an internal balance of the token, mitigating the impact of tokens being forcefully transferred to the contract.

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Note.sol#L13-L19

Vulnerability details

Impact

By leaving _mint_to_Accountant() with no access control when accountant = address(0) it allows an attacker to call the function, mint the entire supply to themselves, and gain the accountant and admin roles. Additionally, the parameter "address accountantDelegator" is expected but never used in the function.

Proof of Concept

Note.sol#L13

Recommended Mitigation Steps

Add admin access control to the _mint_to_Accountant() function

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/CNote.sol#L14-L21

Vulnerability details

Impact

The function does not have access control before the accountant address is set, allowing anyone to call the function, gain admin privileges, and set the accountant address.

Proof of Concept

CNote.sol#L17

Recommended Mitigation Steps

Include access control even when _accountant is not set or set _accountant during deployment

CNote.sol functions borrowFresh() and redeemFresh() do not follow the checks-effects-interactions pattern

1. File CNote.sol#L76

       doTransferOut(borrower, borrowAmount);
	    require(getCashPrior() == 0,"CNote::borrowFresh: Error in doTransferOut, impossible Liquidity in LendingMarket");
	//Amount minted by Accountant is always flashed from account
	
	/* We write the previously calculated values into storage */
        accountBorrows[borrower].principal = accountBorrowsNew;
        accountBorrows[borrower].interestIndex = borrowIndex;
        totalBorrows = totalBorrowsNew;

2. File CNote.sol#L332

       doTransferOut(redeemer, redeemAmount);
	
        /* We write previously calculated values into storage */
        totalSupply = totalSupply - redeemTokens;
        accountTokens[redeemer] = accountTokens[redeemer] - redeemTokens;

        /* We emit a Transfer event, and a Redeem event */
        emit Transfer(redeemer, address(this), redeemTokens);
        emit Redeem(redeemer, redeemAmount, redeemTokens);

        /* We call the defense hook */
        comptroller.redeemVerify(address(this), redeemer, redeemAmount, redeemTokens);

Mitigation: Move all state updates before doTransferOut()

Constructor visibility is unnecessary

Reason: Constructor visibility is ignored since 0.7.0 Mitigation: Remove keyword "public"

1. File Unitroller.sol#L33

constructor() public {
        // Set admin to caller
        admin = msg.sender;
    }