Volt Protocol contest - catchup's results

Inflation Protected Stablecoin.

General Information

Platform: Code4rena

Start Date: 31/03/2022

Pot Size: $75,000 USDC

Total HM: 7

Participants: 42

Period: 7 days

Judge: Jack the Pug

Total Solo HM: 5

Id: 102

League: ETH

Volt Protocol

Findings Distribution

Researcher Performance

Rank: 5/42

Findings: 3

Award: $2,210.90

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Volt Protocol

Findings Information

🌟 Selected for report: cmichel

Also found by: catchup, rayn

Labels

bug

duplicate

2 (Med Risk)

disagree with severity

Awards

1940.0195 USDC - $1,940.02

External Links

Link to GitHub issue

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/RateLimited.sol#L141

Vulnerability details

Impact

_setBufferCap() function sets the bufferCap. It first calls the _updateBufferStored() function which updates the bufferStored variable. However, calculation of bufferStored depends on bufferCap value: Math.min(bufferStored + (rateLimitPerSecond * elapsed), bufferCap); Therefore, first bufferCap should be set to newBufferCap and then _updateBufferStored() should be called.

Proof of Concept

Let's say _setBufferCap() is called with newBufferCap=8000 and oldBufferCap was 10000. Also let's suppose _updateBufferStored() at line 142 sets bufferStored to 900. In this case bufferStored > bufferCap, which should not happen.

Tools Used

Manual analysis

Recommended Mitigation Steps

Call _updateBufferStored() after bufferCap is set with the new value:

function _setBufferCap(uint256 newBufferCap) internal {
    uint256 oldBufferCap = bufferCap;
    bufferCap = newBufferCap;

    _updateBufferStored();

    emit BufferCapUpdate(oldBufferCap, newBufferCap);
}

#0 - ElliotFriedman
2022-04-07T17:01:35Z

This assumes the admin or governor is malicious which is not a correct assumption to make as if the governor is malicious, the whole system is compromised. This is low severity, low risk.

#1 - jack-the-pug
2022-04-21T03:17:25Z

I believe this is a valid issue and it does not assume the admin is malicious. It's just saying the fact the implementation will not work as expected in certain circumstances.

I tend to keep it as Med.

#2 - jack-the-pug
2022-04-21T03:39:05Z

#29 had this issue explained better.

Findings Information

🌟 Selected for report: rayn

Also found by: 0xDjango, 0xkatana, 0xkowloon, BouSalman, CertoraInc, Dravee, Funen, Hawkeye, IllIllI, Jujic, Kenshin, Kthere, Meta0xNull, Sleepy, TerrierLover, async, aysha, berndartmueller, catchup, cccz, cmichel, csanuragjain, danb, defsec, georgypetrov, hake, hubble, kenta, kyliek, pauliax, rfa, robee, sahar, shenwilly, teryanarmen

Awards

125.7841 USDC - $125.78

Labels

bug

QA (Quality Assurance)

External Links

Link to GitHub issue

C4 finding submitted: (risk = 1 (Low Risk)) Missing non-zero address check

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/core/Permissions.sol#L58 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/core/Permissions.sol#L64 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/core/Permissions.sol#L70 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/core/Permissions.sol#L80

Vulnerability details

Impact

Zero address check should be included especially when changing critical addresses.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Add zero address check

C4 finding submitted: (risk = 1 (Low Risk)) Init function is susceptible to front-running

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/core/Core.sol#L20

Vulnerability details

Impact

Init function is susceptible to front-running by an attacker, in which case the contract deployment would be useless.

Proof of Concept

https://github.com/code-423n4/2022-01-livepeer-findings/issues/112

Tools Used

Manual analysis

Recommended Mitigation Steps

Add a control to init() so that only the deployer can call it.

C4 finding submitted: (risk = 1 (Low Risk)) Modifier may not work as expected

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/refs/CoreRef.sol#L29

Vulnerability details

Impact

If a modifier does not execute _ or revert, the function using that modifier will return the default value causing unexpected behaviour. Therefore, in the case of if condition executes false, a function using this modifier would return the default value of the return variable type.

Proof of Concept

https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-modifier

Tools Used

Manual analysis

Recommended Mitigation Steps

All the paths in a modifier must execute _ or revert.

C4 finding submitted: (risk = 0 (Non-critical)) Function return data type different than the underlying variable data type

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/MultiRateLimited.sol#L230 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/MultiRateLimited.sol#L240 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/MultiRateLimited.sol#L250

Vulnerability details

Impact

Functions return values are uint256, whereas the actual underlying data types are uint112 or uint32.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Change the return values to match the underlying data types.

C4 finding submitted: (risk = 0 (Non-critical)) Missing non-zero amount check

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/GlobalRateLimitedMinter.sol#L47 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/RateLimited.sol#L95 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/volt/Volt.sol#L47 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/volt/Volt.sol#L53 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/volt/Volt.sol#L63

Vulnerability details

Impact

It is a good practice to apply non-zero check for amount parameter in functions such as mint, burn, etc. These functions emit events, so if the amount is zero emitted event would be unnecessary. Also, the whole execution can be avoided in such cases which would save gas.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Apply non-zero amount check where necessary

C4 finding submitted: (risk = 0 (Non-critical)) _replenishBuffer() is missing an event

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/RateLimited.sol#L117

Vulnerability details

Impact

_replenishBuffer() should emit an event since _depleteBuffer() does

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Add the appropriate event

C4 finding submitted: (risk = 0 (Non-critical)) Potential approve race condition

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/volt/Volt.sol#L94

Vulnerability details

Impact

Controlling allowances through approve() function can be susceptible to front-running attacks.

Proof of Concept

https://swcregistry.io/docs/SWC-114

Tools Used

Manual analysis

Recommended Mitigation Steps

Consider using increaseAllowance() and decreaseAllowance()

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0v3rf10w, 0xNazgul, 0xkatana, 0xkowloon, CertoraInc, Dravee, Funen, Hawkeye, Jujic, Kenshin, Meta0xNull, Sleepy, TerrierLover, catchup, csanuragjain, defsec, georgypetrov, kenta, okkothejawa, rayn, rfa, robee, saian, samruna

Awards

145.0996 USDC - $145.10

Labels

bug

G (Gas Optimization)

External Links

Link to GitHub issue

Error messages longer than 32 bytes

Lines of code

Vulnerability details

Impact

Error strings longer than 32 bytes are more expensive.

Proof of Concept

https://blog.polymath.network/solidity-tips-and-tricks-to-save-gas-and-reduce-bytecode-size-c44580b218e6#c17b

Tools Used

Manual analysis

Recommended Mitigation Steps

Limit the error strings to 32 bytes max.

Operations can be made unchecked

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/MultiRateLimited.sol#L340 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/RateLimited.sol#L106 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/Timed.sol#L57

Vulnerability details

Impact

These operations can not underflow, so they can be made unchecked to save gas

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Make these operations unchecked

Public state variables can be made private

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/volt/Volt.sol#L11 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/volt/Volt.sol#L13 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/volt/Volt.sol#L15

Vulnerability details

Impact

DOMAIN_SEPARATOR, PERMIT_TYPEHASH and nonces are used only in Volt.sol, hence can be made private. Saves ~4K gas.

Proof of Concept

Tools Used

Remix

Recommended Mitigation Steps

Change their visibility to private.

Move the require check up

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/RateLimited.sol#L103

Vulnerability details

Impact

newBuffer non-zero check can be made right after reading buffer, so that if require fails the operations in between would be avoided.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Move the require to line 97.

Public state variables can be made internal

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/RateLimited.sol#L11 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/RateLimited.sol#L14 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/RateLimited.sol#L17 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/RateLimited.sol#L20 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/RateLimited.sol#L23

Vulnerability details

Impact

MAX_RATE_LIMIT_PER_SECOND, rateLimitPerSecond, lastBufferUsedTime, bufferCap, doPartialAction can be changed to internal.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Change their visibility to internal.

Constant keccak variables can be changed to immutable

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/core/Permissions.sol#L10 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/core/Permissions.sol#L11 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/core/Permissions.sol#L12 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/core/Permissions.sol#L15

Vulnerability details

Impact

If these variables are constant, keccak operation will be performed whenever the variable is used. However, if they are immutable, keccak hashing would be performed just once within the constructor.

Proof of Concept

https://github.com/code-423n4/2021-10-slingshot-findings/issues/3

Tools Used

Manual analysis

Recommended Mitigation Steps

Change the code as: bytes32 public immutable override BURNER_ROLE; bytes32 public immutable override MINTER_ROLE; bytes32 public immutable override PCV_CONTROLLER_ROLE; bytes32 public constant override GOVERN_ROLE = keccak256("GOVERN_ROLE"); bytes32 public immutable override GUARDIAN_ROLE;

constructor() {
    // Appointed as a governor so guardian can have indirect access to revoke ability
    _setupGovernor(address(this));

    _setRoleAdmin(MINTER_ROLE = keccak256("MINTER_ROLE"), GOVERN_ROLE);
    _setRoleAdmin(BURNER_ROLE = keccak256("BURNER_ROLE"), GOVERN_ROLE);
    _setRoleAdmin(PCV_CONTROLLER_ROLE = keccak256("PCV_CONTROLLER_ROLE"), GOVERN_ROLE);
    _setRoleAdmin(GOVERN_ROLE, GOVERN_ROLE);
    _setRoleAdmin(GUARDIAN_ROLE = keccak256("GUARDIAN_ROLE"), GOVERN_ROLE);
}

Bool can be changed to uint256

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/RateLimited.sol#L23 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/refs/OracleRef.sol#L25

Vulnerability details

Impact

Booleans are more expensive than uint256 or any type that takes up a full word because each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled.

Proof of Concept

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol#L23-L27

Tools Used

Manual analysis

Recommended Mitigation Steps

Consider changing bools to uint256

Unused modifier

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/refs/CoreRef.sol#L29

Volt Protocol contest - catchup's results

Inflation Protected Stablecoin.

General Information

Findings Distribution

Researcher Performance

External Links

[M-02] Faulty logic at _setBufferCap() function - $1,940.02

Findings Information

Labels

Awards

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - ElliotFriedman2022-04-07T17:01:35Z

#1 - jack-the-pug2022-04-21T03:17:25Z

#2 - jack-the-pug2022-04-21T03:39:05Z

[Q-30] QA Report - $125.78

Findings Information

Awards

Labels

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

[G-06] Gas Report - $145.10

Findings Information

Awards

Labels

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Lines of code

Vulnerability details

#0 - ElliotFriedman
2022-04-07T17:01:35Z

#1 - jack-the-pug
2022-04-21T03:17:25Z

#2 - jack-the-pug
2022-04-21T03:39:05Z