Canto contest - hake's results

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Governance/GovernorBravoDelegate.sol#L86

Vulnerability details

Impact

Anyone can call execute() and pass in a malicious proposal.

Proof of Concept

There is no access control for the execute() function.

Tools Used

Manual Review.

Recommended Mitigation Steps

Implement access control to execute().

Lines of code

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

Vulnerability details

Impact

getWETHAddress() still relies on same Comp hard coded address.

Unless WETH address is deployed to an address identical to Comps original address the grantCompinternal() function wont work or in a pessimistic scenario an attacker might deploy a malicious token to that address and steal users funds.

Proof of Concept

As stated in the README.md file the changes made to Comptroller.sol were made "to allow for the granting of any generic ERC-20 token as a reward instead of Comp token"

However, the modification in the grantCompInternal() function does not remove the reference to Comp(), it simply renames the parameters while still using the same Comp address at getWETHAddress().

The functionality has not changed at all, the only change made was the renaming from COMP to WETH.

Here is a link to the original Comptroller.sol where the address getCompAddress() is the same as getWETHAddress().

Additionally, the hard coded address in getWETHAddress() doesn't allow "for the granting of any generic ERC-20 token as a reward" only for WETH.

Tools Used

Manual Review.

Recommended Mitigation Steps

As discussed with sponsor I recommend promptly changing the hard coded address. This seems like something really easy to overlook that could lead up to a really costly mistake.

Lines of code

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

Vulnerability details

Impact

Proposals might be falsely deemed as executed when in fact they have not been executed yet.

Proof of Concept

The queue() function sets newProposal.executed = true even though that is not technically true. For the proposal to be executed it needs to be called via the execute() function.

Tools Used

Manual Review.

Recommended Mitigation Steps

Do not automatically set proposal state to executed when queueing as it doesnt reflect actual state of events.

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Governance/GovernorBravoDelegate.sol#L115

Vulnerability details

Impact

state() function cannot view the state from any proposal except for the latest one.

Proof of Concept

require(proposalCount >= proposalId && proposalId > initialProposalId, "GovernorBravo::state: invalid proposal id");

Currently proposalCount needs to be bigger or equal to proposalId. Assuming proposalId is incremented linearly in conjunction with proposalCount, this implies only the most recent proposalId will pass the require() check above. All other proposals will not be able to have their states checked via this function.

Tools Used

Manual Review.

Recommended Mitigation Steps

Change above function to proposalCount <= proposalId (assuming proposalId is set linearly, which currently is not enforced by code).

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Governance/GovernorBravoDelegate.sol#L115

Vulnerability details

Impact

state() function cannot be called to view proposal state if proposalId == 0.

Proof of Concept

There is no check to prevent queueing a proposalId with a value of 0 via the queue() function. However, in the state() function there is a check preventing using a proposalId == 0. For clarity: initialProposalId must be zero according to _initiate(), therefore, proposalId cannot be 0 according to check below.

function state(uint proposalId) public view returns (ProposalState) {
    require(proposalCount >= proposalId && proposalId > initialProposalId, "GovernorBravo::state: invalid proposal id");

Tools Used

Manual Review

Recommended Mitigation Steps

Implement check to preventing queueing a proposalId == 0.

Lines of code

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

Vulnerability details

Impact

Admin can _grantComp() to any address using any amount and drain the contract.

Proof of Concept

If admin key gets compromised there is no timelock, no amount boundaries and no address limitations to prevent the assets to be drained immediately to the attackers address.

Tools Used

Manual Review.

Recommended Mitigation Steps

There is a few suggestions that could help mitigate this issue: Implement timelock for _grantComp() Implement hard coded recipient so funds cannot be arbitrarily sent to any address. Implement a limit to the amount that can be granted.

Here is a reference to a past submission where this issue has been made by team Watchpug: https://github.com/code-423n4/2022-01-insure-findings/issues/271

QA Report

[L-01] Floating pragma

Using a floating pragma (^) might result in the contract being deployed with a version it was not tested with and might result in bugs that affect the contract system negatively.
Locking the pragma (deleting the ^) helps to ensure that contracts do not accidentally get deployed using an outdated compiler version or a version it was not tested with. For example testing Comptroller.sol with 0.8.14, but deploying it 0.8.11 as BaseV1-core.sol uses this version

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Comptroller.sol#L1394 https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-core.sol#L2

[N-01] Delete comment that serves no purpose

Code not used from original BaseV1-core.sol should be removed instead of commented out for better clarity. https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-core.sol#L362

[N-02] Inconsistent naming convention

It is conventional to capitalise constants. Some constants do not follow this convention. https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Governance/GovernorBravoDelegate.sol#L9-L12

[N-03] Error message typo

faialure -> failure. https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-periphery.sol#L463

[N-04] Unclear error message

Please be more clear with error message linked below. https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-periphery.sol#L466

[N-05] Discrepancy in repo/specs vs intended functionality

Copied from the repo: "the modification of Comptroller to allow for the granting of any generic ERC-20 token as a reward instead of Comp token"

However, as confirmed by sponsor Comptroller.sol does not intend to be able to grant "any generic ERC-20 token as a reward", only WETH.

Gas Report

[G-01] for loop gas optimisation

for (uint i = 0; i < routes.length; i++) {
    address pair = pairFor(routes[i].from, routes[i].to, routes[i].stable);
    if (IBaseV1Factory(factory).isPair(pair)) {
        amounts[i+1] = IBaseV1Pair(pair).getAmountOut(amounts[i], routes[i].from);
    }
}

Gas could be saved by:

• Not initialising variable to default value of zero
• Caching array length
• Using a prefix (++i) instead of a postfix (i++)
• Unchecking increment count

Example:

uint length = routes.length;

for (uint i; i < length;) {
    address pair = pairFor(routes[i].from, routes[i].to, routes[i].stable);
    if (IBaseV1Factory(factory).isPair(pair)) {
        amounts[i+1] = IBaseV1Pair(pair).getAmountOut(amounts[i], routes[i].from);
    }
		unchecked { ++i; }
}

https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-periphery.sol#L136

[G-02] Redundant parameter setting

Parameters are always initialed to zero as their default value, therefore assigning them a value of zero is a waist of gas.

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