Canto contest - zzzitron's results

Lines of code

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

Vulnerability details

Impact

One can steal WETH from other users using approve function. Using this, eventually, any amount of ETH deposited in the WETH contract can be stolen.

Proof of Concept

link to gist containing remix replay In this scenario, account1(0x5B38Da6a701c568545dCfcB03FcB875f56beddC4) deposit 1 ether to get 1 WETH. the account2 (0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2) use approve(account1, account2) function to approve themself 1 WETH from account1. Consequently, calls transferFrom(account1, account2, 1ether) to get the 1ether. (Not in the replay) The account1 then, gets the real ether by using withdraw(1ether).

Tools Used

remix

Recommended Mitigation Steps

Add some check for the msg.sender in the approve function.

Lines of code

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

Vulnerability details

Impact

Anyone can add proposal. These proposals are used later to be executed by timelock in GovernorBravoDelegate. (However, it is unclear whether the code works as intended: see the QA report "Upon queue executed true in GovernorBravoDelegate"). Possible exploits:

• overwrite the existing proposals in the Proposal-Store before it is queued to the lending-market
• spam garbage proposal and queue them in the GovernorBravoDelegate to use up the max proposal, which will delay any further proposal/execute

Proof of Concept

This remix replay shows that one can add proposals without any access control.

Tools Used

remix

Recommended Mitigation Steps

Presumably, the OnlyUniGov modifier should be used for this function.

Lines of code

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

Vulnerability details

Impact

The totalSupply does not follow standard, and shows wrong information It is submitted as medium because this information may result in bigger problem who is using this information.

Proof of Concept

One can see In the same example as insecure approve in WETH report (gist link to the remix replay). There should be totalSupply of 1 ether after the account1 depositted 1ether, as one can check with balanceOf(account1), but the totalSupply shows 0.

Tools Used

remix

Recommended Mitigation Steps

One may use the total balance of the ether depositted in the contract as totalBalance.

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

newblockchain QA report

Summary

• It was difficult to work on this project, due to lack of documentation/tests. Sometimes it was not clear what was the intention or intended usage was.
• Also some addresses were hardcoded, without pointing to a valid contract on the mainnet (unigov in lending-market, for example). It was hard to test.

Low

GovernorBravoDelegator cannot be used with GovernorBravoDelegate

• GovernorBravoDeleagtor.sol:39-58 Although there is no through documentation, based on the name, GovernorBravoDelegator should be used with GovernorBravoDelegate as implementation. However, due to lack of corresponding functions, some exposed functions will not work.
  • _acceptInitialAdmin does not exist in GovernorBravoDelegate
  • _initiate() does not exist in GovernorBravoDelegate but only _initiate(address governorAlpha). The number of arguments does not match.

Upon queue executed true in GovernorBravoDelegate

• link When a new proposal is queued, the executed is set to be true. This is checked in the state function and when true, it will return ProposalState.Executed. Which means any proposal which was queued can actually never be executed. However, due to lack of existing tests and time constraints, is not confirmed.

createPair lack of access control in BaseV1-core

• link The function createPair is used to deploy a contract for a token pairs. And any one can create such a pair, also decide to give stable flag true or false. It is unclear whether it was intended usage or not, nor it is tested and confirmed.

Non-Critical

Window should be enforced to be greater than 0 in BaseV1-core

• BaseV1-core.sol:218-235 In the sample function, the argument window should be checked to be greater than 0, otherwise, it might revert from division by zero.

COMP token address for WETH hardcoded in Comptroller

• Comptroller.sol:1465 Although, the comment says that the address is WETH address, the contract in the given address in mainnet is COMP contract.

Misleading comments in Proposal-Store.sol

• Proposal-Store.sol:6-7 The comment above ProposalStore does not describe the contract.

symbol is optional for ERC20 in BaseV1-core

• BaseV1-core.sol:109-113 As the ERC20 standard, symbol is not a must, but an optional function. Also, some token like MKR returns base32 for the symbol which will revert when used for the standard ERC20 interface. It is safer to use either wrapper or try catch.