QA Report

[code423n4]

• Low
  • 1. Outdated compiler
  • 2. Upgradable contracts without GAP can lead a upgrade disaster
  • 3. TreasuryDelegator don't allow to change receive logic
  • 4. Call to external contract without authentication
  • 5. Lack of checks
  • 6. Unsecure state order
  • 7. Use of transfer instead of call
  • 8. Lack of ACK during owner change
  • 9. Owner can deny the service
  • 10. Unsafe ERC20 calls
• Non-Critical
  • 11. Use abstract for base contracts
  • 12. Use hardcoded values

Low

1. Outdated compiler

The pragma version used are:

pragma solidity ^0.8.10;
pragma solidity ^0.8.0;

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

Examples:

• AccountantDelegate.sol#L1
• ERC20Burnable.sol#L4

2. Upgradable contracts without GAP can lead a upgrade disaster

Some contract does not implement a good upgradeable logic.

Proxied contracts MUST include an empty reserved space in storage, usually named __gap, in all the inherited contracts.

For example, if it is an interface, you have to use the interface keyword, otherwise it is a contract that requires your GAP to be correctly updated, so if a new variable is created it could lead in storage collisions that will produce a contract dissaster, including loss of funds.

Reference:

• https://docs.openzeppelin.com/contracts/3.x/upgradeable#storage_gaps
• https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/2cb8996b777060e658e2b8c9b1630313aedb04c0/contracts/token/ERC20/ERC20Upgradeable.sol#L394

Affected source code:

• AccountantErrors
• AccountantDelegatorStorage
• AccountantStorageV1
• AccountantDelegatorInterface
• AccountantInterface
• TreasuryDelegatorStorage
• TreasuryStorageV1
• TreasuryDelegatorInterface
• TreasuryInterface
• GovernorBravoEvents
• GovernorBravoDelegatorStorage
• GovernorBravoDelegateStorageV1
• GovernorBravoDelegateStorageV2

3. TreasuryDelegator don't allow to change receive() logic

The contract does not send the call to the implementation and therefore cannot be updated by an implementation change.

Affected source code:

• TreasuryDelegator.sol#L136

4. Call to external contract without authentication

It is allowed to call the redeem method without authentication control or whitelist, so the contract could call an external contract on behalf of the treasury, something that is not recommended.

Affected source code:

• TreasuryDelegate.sol#L53

5. 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:

• GovernorAlpha.sol#L130
• Comp.sol#L64
• Reservoir.sol#L33

There are also integer variables that must be in a certain range that are not checked.

Affected source code:

• NoteInterest.sol#L165
• NoteInterest.sol#L178
• NoteInterest.sol#L191

6. Unsecure state order

Negative states must be processed before positive states, otherwise, a proposal that is Expired or Executed, but is Active or Pending will be returned as Active or Pending, this makes it necessary to check that the state is correct from outside this method. I mean, changing outside the variables that alter this state in the correct way.

Affected source code:

• GovernorAlpha.sol#L227-L247

function state(uint proposalId) public view returns (ProposalState) {
    require(proposalCount >= proposalId && proposalId > 0, "GovernorAlpha::state: invalid proposal id");
    Proposal storage proposal = proposals[proposalId];
    if (proposal.canceled) {
        return ProposalState.Canceled;
    } else if (block.number <= proposal.startBlock) {
        return ProposalState.Pending;
    } else if (block.number <= proposal.endBlock) {
        return ProposalState.Active;
    } else if (proposal.forVotes <= proposal.againstVotes || proposal.forVotes < quorumVotes()) {
        return ProposalState.Defeated;
    } else if (proposal.eta == 0) {
        return ProposalState.Succeeded;
    } else if (proposal.executed) {
        return ProposalState.Executed;
    } else if (block.timestamp >= add256(proposal.eta, timelock.GRACE_PERIOD())) {
        return ProposalState.Expired;
    } else {
        return ProposalState.Queued;
    }
}

7. Use of transfer instead of call

To transfer ether, the transfer method (which is capped at 2300 gas) is used instead of call which is limited to the gas provided by the user.

If a contract that has a fallback method more expensive than 2300 gas, it will be impossible for a contract receive funds from Basket contract.

Reference:

• transfer -> The receiving smart contract should have a fallback function defined or else the transfer call will throw an error. There is a gas limit of 2300 gas, which is enough to complete the transfer operation. It is hardcoded to prevent reentrancy attacks.
• send -> It works in a similar way as to transfer call and has a gas limit of 2300 gas as well. It returns the status as a boolean.
• call -> It is the recommended way of sending ETH to a smart contract. The empty argument triggers the fallback function of the receiving address.

Affected source code:

• WETH.sol#L31

8. 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:

• Note.sol#L21
• NoteInterest.sol#L113

9. Owner can deny the service

adjusterCoefficient is casted to int and the owner can set a value in _setAdjusterCoefficient that can produce an overflow.

Affected source code:

• NoteInterest.sol#L148

10. 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 transferFrom:

• CNote.sol#L77

Non-Critical

11. Use abstract for base contracts

Abstract contracts are contracts that have at least one function without its implementation. An instance of an abstract cannot be created.

Reference:

• https://docs.soliditylang.org/en/v0.6.2/contracts.html#abstract-contracts

Affected source code:

• AccountantErrors
• AccountantDelegatorStorage
• AccountantStorageV1
• TreasuryDelegatorStorage
• TreasuryStorageV1
• GovernorBravoEvents
• GovernorBravoDelegatorStorage
• GovernorBravoDelegateStorageV1
• GovernorBravoDelegateStorageV2

12. Use hardcoded values

It is not good practice to hardcode values, but if you are dealing with addresses much less, these can change between implementations, networks or projects, so it is convenient to remove these addresses from the source code.

Affected source code:

• GovernorBravoDelegate.sol#L28
• ERC20DirectBalanceManipulation.sol#L10)
• ERC20MaliciousDelayed.sol#L10

[GalloDaSballo]

1. Outdated compiler

Valid R

2. Upgradable contracts without GAP can lead a upgrade disaster

Disagree because the relation being 1-1 means that the only pre-requisite to change implementation is that the new one covers the same slots, adding new slots is not an issue and doesn't need a gap

3. TreasuryDelegator don't allow to change receive() logic

NC

[GalloDaSballo]

4. Call to external contract without authentication

Am also confused by the fact that anyone can call this - Valid Low

5. Lack of checks

L

6. Unsecure state order

In lack of POC i dispute as the example given is not logically possible (e.g. Pending and Expired), please add a POC / example for reports in the future

7. Use of transfer instead of call

L

8. Lack of ACK during owner change

NC

9. Owner can deny the service

L (for now)

10. Unsafe ERC20 calls

Disputed (check assembly below)

11. Use abstract for base contracts

R

12. Use hardcoded values

Disagree, hardcode is fine

[GalloDaSballo]

3L 2R 2NC

[GalloDaSballo]

Titles for Report (Adding the downgraded findings)

L01 - Call to external contract without authentication L02 - Lack of checks L03 - Use of transfer instead of call L04 - Owner can deny the service L05 - CNote.noReentrant backdoor L06 - It is allowed to modify a previously created proposal

NC 01 - Outdated compiler NC 02 - TreasuryDelegator don't allow to change receive() logic NC 03 - Lack of ACK during owner change NC 04 - Use abstract for base contracts