Canto contest - 0x1f8b's results

Lines of code

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

Vulnerability details

Impact

The rand seed used for borrow rate is controlled by the user.

Proof of Concept

In the getBorrowRate method of the NoteInterest contract, a rand value is used for the borrow rate, but it was taken from the msg.sender address, the public address of any EOA could be brute-forced to take advantage of this randomness

    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;        // <------- INSECURE
        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;
    }

Recommended Mitigation Steps

• Change the rand seed origin.

Lines of code

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

Vulnerability details

Impact

There are a lack of governance during proposal management.

Proof of Concept

The AddProposal method of ProposalStore does not contain any restrictions on which user can call it. So anyone can add any proposal, and what is worse, overwriting proposals, since it does not take into account that the proposal already exists.

    constructor(uint propId, string memory title, string memory desc, address[] memory targets, 
                        uint[] memory values, string[] memory signatures, bytes[] memory calldatas) {
	UniGovModAcct = msg.sender;
	Proposal memory prop = Proposal(propId, title, desc, targets, values, signatures, calldatas);
	proposals[propId] = prop;
    }
    
    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;
    }

A malicious user can carry out a front-running attack and replace the propId that is generated during the contract deployment, with different values, so that if the owner does not realize it, a replaced proposal can be approved.

It also allows third parties to deny service, continuously replacing legitimate proposals with attacks.

Recommended Mitigation Steps

• Add OnlyUniGov modifier

Low

Factory can call twice initialize and change the tokens

The method initialize doesn't check that was already initialized. So if the user factory call them again, could produce lose of tokens.

Affected source code:

• UniswapV2Pair.sol#L72

Lack of checks

The following methods have a lack checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code:

address(0):

• UniswapV2Router02.sol#L24
• UniswapV2Pair.sol#L74-L75
• UniswapV2Factory.sol#L19
• UniswapV2Oracle.sol#L53
• BaseV1-periphery.sol#L78
• AccountantDelegator.sol#L17-L22
• AccountantDelegate.sol#L15

Emit event for important state changes

It's important to emit events when the governance change important values in the contract, in order to be more open and transparent.

Affected source code:

• BaseV1-core.sol#L509

Unsafe ERC20 calls

The following code doesn't check the result of the ERC20 calls. ERC20 standard specify that the token can return false if these calls fails, so it's mandatory to check the result of these ERC20 methods.

Reference:

• eip-20

NOTES: The following specifications use syntax from Solidity 0.4.17 (or above) Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!

Affected source code for transfer:

• AccountantDelegate.sol#L87-L89
• TreasuryDelegate.sol#L56
• Comptroller.sol#L1380

Open TODO

The code that contains "open todos" reflects that the development is not finished and that the code can change a posteriori, prior release, with or without audit.

// TODO: Don't distribute supplier COMP if the user is not in the borrower market.:

• Comptroller.sol#L1271

Non Critical

Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

• UniswapV2Factory.sol#L59

Wrong comments

In the contract ProposalStore there are copy/pasted comment:

Affected source code:

// This is an evil token. Whenever an A -> B transfer is called, half of the amount goes to B and half to a predefined C:

• Proposal-Store.sol#L6-L7

reserveFactorMantissa not used

Affected source code:

• NoteInterest.sol#L109

Use right type

Use bool for unlocked and move this var along with blockTimestampLast to share the same storage slot:

Affected source code:

• UniswapV2Pair.sol#L36
• BaseV1-core.sol#L123

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

If it's not possible to use error codes due to the pragma used, it is recommended to reduce the strings to less than 32 bytes.

All contracts in scope are affected.

Obsolete pragma

The pragma version used in uniswap contracts are:

pragma solidity =0.6.12;:

But recently solidity released a new version with important Bugfixes:

• The first one is related to ABI-encoding nested arrays directly from calldata. You can find more information here.

• The second bug is triggered in certain inheritance structures and can cause a memory pointer to be interpreted as a calldata pointer or vice-versa. We also have a dedicated blog post about this bug.

Apart from these, there are several minor bug fixes and improvements.

The minimum required version should be 0.8.14

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

It is also recommended to not initialize the counter variable and surround the increment with an unchecked region.

Affected source code:

• UniswapV2Oracle.sol#L83
• UniswapV2Router02.sol#L323
• BaseV1-core.sol#L207
• BaseV1-core.sol#L337
• BaseV1-periphery.sol#L136
• BaseV1-periphery.sol#L362
• GovernorBravoDelegate.sol#L68
• GovernorBravoDelegate.sol#L90
• Comptroller.sol#L126
• Comptroller.sol#L206
• Comptroller.sol#L735
• Comptroller.sol#L959
• Comptroller.sol#L1005
• Comptroller.sol#L1106
• Comptroller.sol#L1364
• Comptroller.sol#L1413

Use constants

Use constant instead of storage for:

• ERC20MaliciousDelayed.sol#L10-L11
• ERC20DirectBalanceManipulation.sol#L10

Cache keccak256 results:

• TreasuryDelegate.sol#L51-L55

Dead code

The following contracts are never used. It's an excellent idea to take them out to save gas and improve the codding style.

Affected source code:

OnlyUniGov and UniGovModAcct is not used:

• Proposal-Store.sol#L30-L35

Solidity 8 already does the overflow/underflow checks:

• GovernorBravoDelegate.sol#L180-L189

• Comptroller.sol#L399-L407

• Comptroller.sol#L454-L463

• Comptroller.sol#L528-L538

• Comptroller.sol#L592-L601

• Comptroller.sol#L640-L648

Gas saving using immutable.

It's possible to avoid storage access a save gas using immutable keyword for the following variables:

Affected source code:

name and symbol:

• BaseV1-core.sol#L39-L40
• WETH.sol#L6-L8

Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

Affected source code:

• BaseV1-core.sol#L232
• BaseV1-periphery.sol#L134

There's no need to set default values for variables.

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Affected source code:

• BaseV1-core.sol#L490
• Comptroller.sol#L947-L948

Use require instead of assert

The assert() and require() functions are a part of the error handling aspect in Solidity. Solidity makes use of state-reverting error handling exceptions. This means all changes made to the contract on that call or any sub-calls are undone if an error is thrown. It also flags an error.

They are quite similar as both check for conditions and if they are not met, would throw an error.

The big difference between the two is that the assert() function when false, uses up all the remaining gas and reverts all the changes made.

Meanwhile, a require() function when false, also reverts back all the changes made to the contract but does refund all the remaining gas fees we offered to pay. This is the most common Solidity function used by developers for debugging and error handling.

Affected source code:

• BaseV1-periphery.sol#L82
• BaseV1-periphery.sol#L227
• BaseV1-periphery.sol#L273
• BaseV1-periphery.sol#L419

Reduce boolean comparison

It's compared a boolean value using == true or == false, instead of using the boolean value, or NOT opcode, it's cheaper to use NOT when the value it's false, or just the value without == true, when it's true, because it will use less opcode inside the VM.

Affected source code:

• Comptroller.sol#L1053
• Comptroller.sol#L1063
• Comptroller.sol#L1072
• Comptroller.sol#L1081
• Comptroller.sol#L1350
• Comptroller.sol#L1357
• Comptroller.sol#L1456

Senseless return

The return of the following methods is the same value as the argument received by the user, so it can be eliminated.

Affected source code:

• Comptroller.sol#L1050-L1086