Canto contest - WatchPug's results

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/b93e2867a64b420ce6ce317f01c7834a7b6b17ca/contracts/NoteInterest.sol#L118-L129

Vulnerability details

function updateBaseRate(uint newBaseRatePerYear) public {
    // check the current block number
    uint blockNumber = block.number;
    uint deltaBlocks = blockNumber.sub(lastUpdateBlock);

    if (deltaBlocks > updateFrequency) {
        // pass in a base rate per year
        baseRatePerYear = newBaseRatePerYear;
        lastUpdateBlock = blockNumber;
        emit NewInterestParams(baseRatePerYear);
    }
}

Recommendation

Add proper access control.

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/b93e2867a64b420ce6ce317f01c7834a7b6b17ca/contracts/WETH.sol#L85-L88

Vulnerability details

function approve(address owner, address spender) external returns(bool) {
    _approve(owner, spender, _balanceOf[owner]);
    return true;
}

approve(address owner, address spender) is a public function with no access control, the attacker can call it and approve from an arbitrary address (owner) to their own address (spender) and empty their balances.

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/b93e2867a64b420ce6ce317f01c7834a7b6b17ca/contracts/Accountant/AccountantDelegate.sol#L74-L92

Vulnerability details

function sweepInterest() external override returns(uint) {
	uint noteBalance = note.balanceOf(address(this));
	uint CNoteBalance = cnote.balanceOf(address(this));

	Exp memory expRate = Exp({mantissa: cnote.exchangeRateStored()}); // obtain exchange Rate from cNote Lending Market as a mantissa (scaled by 1e18)
	uint cNoteConverted = mul_ScalarTruncate(expRate, CNoteBalance); //calculate truncate(cNoteBalance* mantissa{expRate})
	uint noteDifferential = sub_(note.totalSupply(), noteBalance); //cannot underflow, subtraction first to prevent against overflow, subtraction as integers

	require(cNoteConverted >= noteDifferential, "Note Loaned to LendingMarket must increase in value");
	
	uint amtToSweep = sub_(cNoteConverted, noteDifferential);

	note.transfer(treasury, amtToSweep);

	cnote.transfer(address(0), CNoteBalance);

	return 0;
}

cnote.transfer(address(0), CNoteBalance); will burn all the balance in the Accountant contract, which makes no sense and will cause the Accountant contract to lose most of the funds and malfunction ever since.

Recommendation

Remove cnote.transfer(address(0), CNoteBalance);.

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/b93e2867a64b420ce6ce317f01c7834a7b6b17ca/contracts/NoteInterest.sol#L92-L101

Vulnerability details

function getBorrowRate(uint cash, uint borrows, uint reserves) public view override returns (uint) {
    // Gets the Note/gUSDC TWAP in a given interval, as a mantissa (scaled by 1e18)
    // uint twapMantissa = getUnderlyingPrice(note);
    uint rand = uint(keccak256(abi.encodePacked(msg.sender))) % 100;
    uint ir = (100 - rand).mul(adjusterCoefficient).add(baseRatePerYear).mul(1e16);
    uint newRatePerYear = ir >= 0 ? ir : 0;
    // convert it to base rate per block
    uint newRatePerBlock = newRatePerYear.div(blocksPerYear);
    return newRatePerBlock;
}

The current implementation will return a random rate based on the caller's address and baseRatePerYear.

This makes some lucky addresses pay much lower and some addresses pay much higher rates.

Lines of code

https://github.com/Plex-Engineer/zeroswap/blob/03507a80322112f4f3c723fc68bed0f138702836/contracts/uniswapv2/libraries/UniswapV2Library.sol#L20-L28

Vulnerability details

function pairFor(address factory, address tokenA, address tokenB) internal pure returns (address pair) {
    (address token0, address token1) = sortTokens(tokenA, tokenB);
    pair = address(uint(keccak256(abi.encodePacked(
            hex'ff',
            factory,
            keccak256(abi.encodePacked(token0, token1)),
            hex'e18a34eb0e04b04f7a0ac29a6e80748dca96319b42c54d679cb821dca90c6303' // init code hash
        ))));
}

The init code hash in UniswapV2Library.pairFor() should be updated since the code of UniswapV2Pair has been changed. Otherwise, the pair address calculated will be wrong, most likely non-existing address.

There are many other functions and other contracts across the codebase, including UniswapV2Oracle, UniswapV2Router02, and SushiRoll, that rely on the UniswapV2Library.pairFor() function for the address of the pair, with the UniswapV2Library.pairFor() returning a wrong and non-existing address, these functions and contracts will malfunction.

Recommendation

Update the init code hash from hex'e18a34eb0e04b04f7a0ac29a6e80748dca96319b42c54d679cb821dca90c6303' to the value of UniswapV2Factory.pairCodeHash().

Lines of code

https://github.com/Plex-Engineer/manifest/blob/688e9b4e7835854c22ef44b045d6d226b784b4b8/contracts/Proposal-Store.sol#L46-L50

Vulnerability details

function AddProposal(uint propId, string memory title, string memory desc, address[] memory targets, 
                    uint[] memory values, string[] memory signatures, bytes[] memory calldatas) public {
    Proposal memory newProp = Proposal(propId, title, desc, targets, values, signatures, calldatas);
    proposals[propId] = newProp;
}

Proposal-Store.sol#AddProposal() is a public function, and anyone call add a new proposal with arbitrary properties, or update a previous proposal's properties.

The malicious proposal can then be queued and executed on lending-market/GovernorBravoDelegate and so.

https://github.com/Plex-Engineer/lending-market/blob/b93e2867a64b420ce6ce317f01c7834a7b6b17ca/contracts/Governance/GovernorBravoDelegate.sol#L37-L75

function queue(uint proposalId) external {
    // Query the proposal from the unigov map contract
    IProposal.Proposal memory unigovProposal = unigov.QueryProp(proposalId);

    // Allow addresses above proposal threshold and whitelisted addresses to propose
    require(unigovProposal.targets.length == unigovProposal.values.length && 
            unigovProposal.targets.length == unigovProposal.signatures.length && 
            unigovProposal.targets.length == unigovProposal.calldatas.length, 
            "GovernorBravo::propose: proposal function information arity mismatch");
    require(unigovProposal.targets.length != 0, "GovernorBravo::propose: must provide actions");
    require(unigovProposal.targets.length <= proposalMaxOperations, "GovernorBravo::propose: too many actions");

    // Add proposal to proposals storage
    Proposal storage newProposal = proposals[unigovProposal.id];

    // Make sure you are not overriding an existing proposal 
    require(proposals[unigovProposal.id].id == 0);

    // Set newProposal to the fields of unigov proposal
    newProposal.id = unigovProposal.id;
    newProposal.eta = 0;
    newProposal.targets = unigovProposal.targets;
    newProposal.values = unigovProposal.values;
    newProposal.signatures = unigovProposal.signatures;
    newProposal.calldatas = unigovProposal.calldatas;
    newProposal.canceled = false;
    newProposal.executed = true;

    uint eta = add256(block.timestamp, timelock.delay());
    

    for (uint i = 0; i < newProposal.targets.length; i++) {
        queueOrRevertInternal(newProposal.targets[i], newProposal.values[i], newProposal.signatures[i], newProposal.calldatas[i], eta);
    }

    newProposal.eta = eta;

    emit ProposalQueued(proposalId, eta);
}

Recommendation

Add proper access control.

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/b93e2867a64b420ce6ce317f01c7834a7b6b17ca/contracts/CNote.sol#L31-L87

Vulnerability details

function borrowFresh(address payable borrower, uint borrowAmount) internal override {
    /* Fail if borrow not allowed */
    uint allowed = comptroller.borrowAllowed(address(this), borrower, borrowAmount);
    if (allowed != 0) {
        revert BorrowComptrollerRejection(allowed);
    }

    /* Verify market's block number equals current block number */
    if (accrualBlockNumber != getBlockNumber()) {
        revert BorrowFreshnessCheck();
    }

    require(getCashPrior() == 0, "CNote::borrowFresh:Impossible reserves in CNote market Place");

    require(address(_accountant) != address(0), "CNote::borrowFresh:Accountant has not been initialized");

    /*Protocol always has a balance of 0 Note, thus the accountant must mint borrowAmount tokens */
    uint err = _accountant.supplyMarket(borrowAmount);
    
    if (err != 0) {
        revert AccountantSupplyError(borrowAmount);
    }

    require(getCashPrior() == borrowAmount, "CNote::borrowFresh:Error in Accountant supply");


    /*
        * We calculate the new borrower and total borrow balances, failing on overflow:
        *  accountBorrowsNew = accountBorrows + borrowAmount
        *  totalBorrowsNew = totalBorrows + borrowAmount
        */
    uint accountBorrowsPrev = borrowBalanceStoredInternal(borrower);
    uint accountBorrowsNew = accountBorrowsPrev + borrowAmount;
    uint totalBorrowsNew = totalBorrows + borrowAmount;

    /////////////////////////   
    // EFFECTS & INTERACTIONS
    // (No safe failures beyond this point)

    /*
        * We invoke doTransferOut for the borrower and the borrowAmount.
        *  Note: The cToken must handle variations between ERC-20 and ETH underlying.
        *  On success, the cToken borrowAmount less of cash.
        *  doTransferOut reverts if anything goes wrong, since we can't be sure if side effects occurred.
        */
    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;

    /* We emit a Borrow event */
    emit Borrow(borrower, borrowAmount, accountBorrowsNew, totalBorrowsNew);
}

Both borrowFresh() and repayBorrowFresh() checks and requires getCashPrior() == 0, which mean the balance of NOTE must be 0, otherwise, the borrowFresh() and repayBorrowFresh() will revert with an error.

However, anyone can send 1 wei of NOTE token to the CNote contract, and break the two most essential functions of the cNote contract.

Recommendation

Consider changing the checks and do not rely on balanceOf().

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/b93e2867a64b420ce6ce317f01c7834a7b6b17ca/contracts/WETH.sol#L46-L48

Vulnerability details

function totalSupply() public view returns (uint) {
        return _balanceOf[address(this)];
    }

The totalSupply is supposed to return the total supply of the token.

In the current implementation, it's returning the balance of the contract itself, which will always be 0 unless tokens get sent to the contract for some reason (that is quite unlikely to happen, because doing that means losing the funds).

Recommendation

Change to:

function totalSupply() public view returns (uint) {
    return address(this).balance;
}

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/b93e2867a64b420ce6ce317f01c7834a7b6b17ca/contracts/Comptroller.sol#L1376-L1384

Vulnerability details

function grantCompInternal(address user, uint amount) internal returns (uint) {
    WETH comp = WETH(payable(getWETHAddress())); 
    uint compRemaining = comp.balanceOf(address(this));
    if (amount > 0 && amount <= compRemaining) {
        comp.transfer(user, amount);
        return 0;
    }
    return amount;
}

https://github.com/Plex-Engineer/lending-market/blob/b93e2867a64b420ce6ce317f01c7834a7b6b17ca/contracts/Comptroller.sol#L1469-L1471

function getWETHAddress() virtual public view returns (address) {
    return 0xc00e94Cb662C3520282E6f5717214004A7f26888;
}

While the function name of getCompAddress() get changed to getWETHAddress(), the address itself (0xc00e94Cb662C3520282E6f5717214004A7f26888) remain unchanged.

As a result, the compRemaining in grantCompInternal() will always be 0.

Recommendation

Consider adding an immutable variable to the constructor for WETH.

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/b93e2867a64b420ce6ce317f01c7834a7b6b17ca/contracts/Note.sol#L13-L31

Vulnerability details

function _mint_to_Accountant(address accountantDelegator) external {
    if (accountant == address(0)) {
        _setAccountantAddress(msg.sender);
    }
    require(msg.sender == accountant, "Note::_mint_to_Accountant: ");
    _mint(msg.sender, type(uint).max);
}

function RetAccountant() public view returns(address) {
    return accountant;
}

function _setAccountantAddress(address accountant_) internal {
    if(accountant != address(0)) {
        require(msg.sender == admin, "Note::_setAccountantAddress: Only admin may call this function");
    }
    accountant = accountant_;
    admin = accountant;
}

_mint_to_Accountant() calls _setAccountantAddress() when accountant == address(0), which will always be the case when _mint_to_Accountant() is called for the first time.

And _setAccountantAddress() only checks if msg.sender == admin when accountant != address(0) which will always be false, therefore the access control is not working.

L17 will then check if msg.sender == accountant, now it will always be the case, because at L29, accountant was set to msg.sender.

Lines of code

https://github.com/Plex-Engineer/zeroswap/blob/03507a80322112f4f3c723fc68bed0f138702836/contracts/uniswapv2/UniswapV2Pair.sol#L125-L137

Vulnerability details

if (_totalSupply == 0) {
    address migrator = IUniswapV2Factory(factory).migrator();
    if (msg.sender == migrator) {
        liquidity = IMigrator(migrator).desiredLiquidity();
        require(liquidity > 0 && liquidity != uint256(-1), "Bad desired liquidity");
    } else {
        require(migrator == address(0), "Must not have migrator");
        liquidity = Math.sqrt(amount0.mul(amount1)).sub(MINIMUM_LIQUIDITY);
        _mint(address(0), MINIMUM_LIQUIDITY); // permanently lock the first MINIMUM_LIQUIDITY tokens
    }
} else {
    liquidity = Math.min(amount0.mul(_totalSupply) / _reserve0, amount1.mul(_totalSupply) / _reserve1);
}

https://github.com/Plex-Engineer/zeroswap/blob/03507a80322112f4f3c723fc68bed0f138702836/contracts/Migrator.sol#L28-L46

function migrate(IUniswapV2Pair orig) public returns (IUniswapV2Pair) {
    require(msg.sender == chef, "not from master chef");
    require(block.number >= notBeforeBlock, "too early to migrate");
    require(orig.factory() == oldFactory, "not from old factory");
    address token0 = orig.token0();
    address token1 = orig.token1();
    IUniswapV2Pair pair = IUniswapV2Pair(factory.getPair(token0, token1));
    if (pair == IUniswapV2Pair(address(0))) {
        pair = IUniswapV2Pair(factory.createPair(token0, token1));
    }
    uint256 lp = orig.balanceOf(msg.sender);
    if (lp == 0) return pair;
    desiredLiquidity = lp;
    orig.transferFrom(msg.sender, address(orig), lp);
    orig.burn(address(pair));
    pair.mint(msg.sender);
    desiredLiquidity = uint256(-1);
    return pair;
}

When minting LP tokens (addLiquidity), the amount of lp tokens you are getting is calculated based on liquidity = Math.min(amount0.mul(_totalSupply) / _reserve0, amount1.mul(_totalSupply) / _reserve1);, if the _totalSupply is small enough, and 1 wei of the lp token worth large amounts of token0 and token1, the user who adds small amounts of liquidity will receive less amount of lp tokens due to precision loss.

A sophisticated attacker can artificially create that scenario by mint only 1 wei of lp token and add 1e24 or even larger amounts of token0 and token1 by sending the tokens to the contract and then call sync() to update the reserves.

Then all the new depositors will lose up to 1e24, let's say they deposited 1.99e24, they will only receive 1 wei of lp token, therefore, losing 0.99e24 of token0 and token1.

This attack vector was mitigated the original version of UniswapV2Pair by introcuing the MINIMUM_LIQUIDITY minted and permanently lock in address(0) upon the first mint.

However, this can now be bypassed with the migrator, and this attacker vector is open again.

Recommendation

Given the fact that zeroswap will be a DEX that does not need a feature to migrate liquidity from other DEXs, consider removing the migrator.

Lines of code

https://github.com/Plex-Engineer/stableswap/blob/0dd7ac65d923bb7462c47f6d56b564af34b34118/contracts/BaseV1-core.sol#L154-L171

Vulnerability details

function _update(uint balance0, uint balance1, uint _reserve0, uint _reserve1) internal {
    uint blockTimestamp = block.timestamp;
    uint timeElapsed = blockTimestamp - blockTimestampLast; // overflow is desired
    if (timeElapsed > 0 && _reserve0 != 0 && _reserve1 != 0) {
        reserve0CumulativeLast += _reserve0 * timeElapsed;
        reserve1CumulativeLast += _reserve1 * timeElapsed;
    }

    Observation memory _point = lastObservation();
    timeElapsed = blockTimestamp - _point.timestamp; // compare the last observation with current timestamp, if greater than 30 minutes, record a new event
    if (timeElapsed > periodSize) {
        observations.push(Observation(blockTimestamp, reserve0CumulativeLast, reserve1CumulativeLast));
    }
    reserve0 = balance0;
    reserve1 = balance1;
    blockTimestampLast = blockTimestamp;
    emit Sync(reserve0, reserve1);
}

This was forked from Uniswap v2's update():

https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2Pair.sol#L72-L81

// update reserves and, on the first call per block, price accumulators
function _update(uint balance0, uint balance1, uint112 _reserve0, uint112 _reserve1) private {
    require(balance0 <= uint112(-1) && balance1 <= uint112(-1), 'UniswapV2: OVERFLOW');
    uint32 blockTimestamp = uint32(block.timestamp % 2**32);
    uint32 timeElapsed = blockTimestamp - blockTimestampLast; // overflow is desired
    if (timeElapsed > 0 && _reserve0 != 0 && _reserve1 != 0) {
        // * never overflows, and + overflow is desired
        price0CumulativeLast += uint(UQ112x112.encode(_reserve1).uqdiv(_reserve0)) * timeElapsed;
        price1CumulativeLast += uint(UQ112x112.encode(_reserve0).uqdiv(_reserve1)) * timeElapsed;
    }

UniswapV2's Pair is using Solidity 0.5.16, in which the arithmetic operations will overflow/underflow without revert.

As the solidity version used in the current implementation of BaseV1Pair.sol is 0.8.11, and there are some breaking changes in Solidity v0.8.0, including:

Arithmetic operations revert on underflow and overflow.

Ref: https://docs.soliditylang.org/en/v0.8.11/080-breaking-changes.html#silent-changes-of-the-semantics

When updating reserve0CumulativeLast and reserve1CumulativeLast in BaseV1Pair.sol, overflow and underflow are desired as per the comment.

However, the intended overflow only works for solidity < 0.8.0 by default. If overflow and underflow are desired, then the math should be put into an unchecked block. Otherwise, the transaction will revert.

Impact

Since the overflow is desired in the original version, and it's broken because of using Solidity version >0.8. The BaseV1Pair contract will break when the desired overflow happens, which will be sooner or later depending on the decimals of the tokens and trading volume.

Recommendation

Change to:

unchecked {
    uint timeElapsed = blockTimestamp - blockTimestampLast;
    if (timeElapsed > 0 && _reserve0 != 0 && _reserve1 != 0) {
        reserve0CumulativeLast += _reserve0 * timeElapsed;
        reserve1CumulativeLast += _reserve1 * timeElapsed;
    }
}