Canto contest - Ruhum's results

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/main/contracts/Accountant/AccountantDelegate.sol#L29

Vulnerability details

Impact

It's not possible to initialize the accountant because of a mistake in the function's require statement.

I rate it as MED since a key part of the protocol wouldn't be available until the contract is modified and redeployed.

Proof of Concept

The issue is the following require() statement: https://github.com/Plex-Engineer/lending-market/blob/main/contracts/Accountant/AccountantDelegate.sol#L29

There, the function checks whether the accountant has received the correct amount of tokens. But, it compares the accountant's balance with the _initialSupply. That value is always 0. So the require statement will always fail

When the Note contract is initialized, _initialSupply is set to 0:

• https://github.com/Plex-Engineer/lending-market/blob/main/deploy/canto/004_deploy_Note.ts#L14
• https://github.com/Plex-Engineer/lending-market/blob/main/contracts/Note.sol#L9
• https://github.com/Plex-Engineer/lending-market/blob/main/contracts/ERC20.sol#L32

After _mint_to_Accountant() mints type(uint).max tokens to the accountant: https://github.com/Plex-Engineer/lending-market/blob/main/contracts/Note.sol#L18 That increases the totalSupply but not the _initialSupply: https://github.com/Plex-Engineer/lending-market/blob/main/contracts/ERC20.sol#L242

The _initialSupply value is only modified by the ERC20 contract's constructor.

Tools Used

none

Recommended Mitigation Steps

Change the require statement to

require(note.balanceOf(msg.sender) == note.totalSupply(), "AccountantDelegate::initiatlize: Accountant has not received payment");

Lines of code

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

Vulnerability details

Impact

The ProposalStore contract's AddProposal() function is callable by anyone and allows the caller to overwrite an existing proposal.

This can be abused to block proposals from being queued and executed properly.

Proof of Concept

The governance process goes through three steps:

1. create a proposal through ProposalStore.AddProposal()
2. queue the proposal through GovernorBravoDelegate.queue()
3. execute the proposal through GovernorBravoDelegate.execute()

The attacker is able to overwrite existing proposals in the ProposalStore contract. They simply call AddProposal() with propId being the proposal they want to overwrite. The function doesn't check whether a proposal with that ID already exists or not. It simply overwrites it in the proposals map.

So the attacker simply watches the mempool for any transaction where GovernorBravoDelegate.queue() is executed and front runs it with a transaction where that specific proposal is modified. The user then queues a malicious proposal which will be canceled before the execution delay is over.

The proposer is able to bypass this issue by creating and queuing a proposal within the same transaction. Or they submit both transactions to a miner directly instead of going through the public mempool. Because of that, I'd rate it as a MED issue.

Tools Used

none

Recommended Mitigation Steps

Don't allow existing proposals from being overwritten.

Judge has assessed an item in Issue #54 as High risk. The relevant finding follows:

Lines of code

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

Vulnerability details

Impact

The Comptroller contract uses a hardcoded address for the WETH contract which is not the correct one. Because of that, it will be impossible to claim COMP rewards. That results in a loss of funds so I rate it as HIGH.

Proof of Concept

The Comptroller's getWETHAddress() function: https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Comptroller.sol#L1469

It's a left-over from the original compound repo: https://github.com/compound-finance/compound-protocol/blob/master/contracts/Comptroller.sol#L1469

It's used by the grantCompInternal() function: https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Comptroller.sol#L1377

That function is called by claimComp(): https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Comptroller.sol#L1365

If there is a contract stored in that address and it doesn't adhere to the interface (doesn't have a balanceOf() and transfer() function), the transaction will revert. If there is no contract, the call will succeed without having any effect. In both cases, the user doesn't get their COMP rewards.

Tools Used

none

Recommended Mitigation Steps

The WETH contract's address should be parsed to the Comptroller through the constructor or another function instead of being hardcoded.

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/main/contracts/Governance/GovernorBravoDelegate.sol#L63 https://github.com/Plex-Engineer/lending-market/blob/main/contracts/Governance/GovernorBravoDelegate.sol#L87

Vulnerability details

Impact

It's not possible to execute a proposal through the GovernorBravoDelegate contract because the executed property of it is set to true when it's queued up.

Since this means that the governance contract is unusable, it might result in locked-up funds if those were transferred to the contract before the issue comes up. Because of that I'd rate it as HIGH.

Proof of Concept

executed is set to true: https://github.com/Plex-Engineer/lending-market/blob/main/contracts/Governance/GovernorBravoDelegate.sol#L63

Here, the execute() function checks whether the proposal's state is Queued: https://github.com/Plex-Engineer/lending-market/blob/main/contracts/Governance/GovernorBravoDelegate.sol#L87 But, since the execute property is true, the state() function will return Executed: https://github.com/Plex-Engineer/lending-market/blob/main/contracts/Governance/GovernorBravoDelegate.sol#L117

In the original compound repo, executed is false when the proposal is queued up: https://github.com/compound-finance/compound-protocol/blob/master/contracts/Governance/GovernorBravoDelegate.sol#L111

Tools Used

none

Recommended Mitigation Steps

Just delete the line where executed is set to true. Since the zero-value is false anyway, you'll save gas as well.

Report

• 01: checking whether a uint is >= 0 is unnecessary since it's always true
• 02: WETH.totalSupply() provides the incorrect value
• 03: Attacker can frontrun Note._mint_to_accountant()

01: checking whether a uint is >= 0 is unnecessary since it's always true

Relevant code:

• https://github.com/Plex-Engineer/lending-market/blob/main/contracts/NoteInterest.sol#L97

02: WETH.totalSupply() provides the incorrect value

The WETH contract returns its own token balance instead of the total supply of tokens in circulation.

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/WETH.sol#L47

Instead of _balanceOf[address(this)]; it should be this.balance.

Since the function isn't used anywhere it's a spec issue. So I rate it as LOW. Could cause issues in the future if someone depends on the total supply.

03: Attacker can frontrun Note._mint_to_accountant()

By calling the _mint_to_accountant() function first, they mint the tokens to themselve and set their address as both the accountant and admin. The contract would have to be redeployed.

Relevant code:

• https://github.com/Plex-Engineer/lending-market/blob/main/contracts/Note.sol#L13
• https://github.com/Plex-Engineer/lending-market/blob/main/deploy/canto/004_deploy_Note.ts#L12 (not called after deployment)

Gas Report

• 01: Don't assign the zero-value when initializing structs
• 02: don't use custom math functions to prevent overflows
• 03: don't check for possible underflows

01: Don't assign the zero-value when initializing structs

The struct has the property's zero-value by default. Assigning it explicitly is just wasting gas.

Relevant code:

• https://github.com/Plex-Engineer/lending-market/blob/main/contracts/Governance/GovernorBravoDelegate.sol#L57
• https://github.com/Plex-Engineer/lending-market/blob/main/contracts/Governance/GovernorBravoDelegate.sol#L62

02: don't use custom math functions to prevent overflows

Solidity comes with built-in safe math after v0.8.0. Having your own functions is just a waste of gas since it will check for overflows twice.

Relevant code:

• https://github.com/Plex-Engineer/lending-market/blob/main/contracts/Governance/GovernorBravoDelegate.sol#L180-L189
• https://github.com/Plex-Engineer/lending-market/blob/main/contracts/Governance/GovernorBravoDelegate.sol#L65
• https://github.com/Plex-Engineer/lending-market/blob/main/contracts/Governance/GovernorBravoDelegate.sol#L119

03: don't check for possible underflows

Since under- & overflow protection is built-in since Solidity v0.8.0 it's unnecessary to check for that. It's a waste of gas.

Relevant code:

• https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/WETH.sol#L29
• https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/WETH.sol#L69
• https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/WETH.sol#L72

There are probably a lot more places where this is relevant. Those are just some that caught my eye.