Platform: Code4rena
Start Date: 11/12/2023
Pot Size: $90,500 USDC
Total HM: 29
Participants: 127
Period: 17 days
Judge: TrungOre
Total Solo HM: 4
Id: 310
League: ETH
Rank: 108/127
Findings: 1
Award: $20.82
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: SBSecurity
Also found by: 0xaltego, 0xbepresent, Aymen0909, Bauchibred, Cosine, EVDoc, EloiManuel, HighDuty, Sathish9098, Tendency, Timeless, ZanyBonzy, beber89, deliriusz, ether_sky, grearlake, hals, klau5, lsaudit, nadin, rvierdiiev, tsvetanovv
20.8157 USDC - $20.82
| QA | Issue |
|---|---|
| [L-01] | Function and Event Arguments With Different Order |
| [L-02] | State Variables not Limited to Reasonable Values |
| [L-03] | Incorrect NatSpec |
| [L-04] | Panic Used for Error Handling |
| [L-05] | Removing an account from canContractExceedMaxDelegates does not Decrease its Number of Delegates if Exceeded |
| [NC-01] | Unnecessary Casting |
| [NC-02] | Implicit Casting |
| [NC-03] | Inconsistent uint Types in Events |
| [NC-04] | Typos in Comments |
The LendingTerm.sol and ERC20Gauges.sol files contain some functions with parameters that emit events containing the same arguments but with a different order.
Example from ERC20Gauges.sol:
function _addGauge( uint256 _type, address gauge ) internal returns (uint256 weight) { ... emit AddGauge(gauge, _type); }
Instances:
The highlighted inconsistencies could difficult traceability of events.
For consistency sake, make sure that arguments of functions that are also used by events are passed/emitted in the same order.
Consider adding appropriate minimum/maximum value checks to ensure that the following state variables can never be used to excessively harm users, including via griefing. Even though the bot report contains some instances for this issue already (see L-11: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/bot-report.md#l-11-state-variables-not-limited-to-reasonable-values), it missed others like:
The NatSpec of the file ERC20Gauges.sol at https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20Gauges.sol#L7-L57 is not accurate.
For instance, it indicates "Rename calculateGaugeAllocation to calculateGaugeStoredAllocation to make clear that it reads from stored weights." and "Consistency: make incrementGauges return a uint112 instead of uint256" even though calculateGaugeStoredAllocation() does not exist and incrementGauges() is still returning uint256.
See similar low findings from past contests:
Suggest removing/updating the referenced comments and/or update the code accordingly.
In the line below, the code panics when an arithmetic overflow or underflow occurs. Panics should be reserved for programmer errors (e.g., assertion violations). Panicking on user errors dilutes the utility of the panic operation.
Similar low risk issues from other audits:
As a proof of concept, you can execute the testDecrementOverWeightFails() test function of the ERC20MultiVotes.t.sol test file by running forge test --mc ERC20MultiVotesUnitTest --silent -vvvv --mt "\btestDecrementOverWeightFails\b". You should see that the execution is reverted with a "Panic" error as expected by the test.
Instead of using panics, custom errors should be defined and handled according to best practices.
canContractExceedMaxDelegates does not Decrease its Number of Delegates if ExceededThe abstract contract ERC20MultiVotes is an ERC20 extension that allows delegations to multiple delegatees up to a user's balance on a given block. As indicated in the NatSpec, maxDelegates is a critical variable and it defines the maximum amount of addresses that users can delegate to. _setContractExceedMaxDelegates() is an internal function that allows certain addresses to bypass the maxDelegates limitation. The CreditToken and GuildToken contracts make use of it and protect it with the CREDIT_GOVERNANCE_PARAMETERS and GUILD_GOVERNANCE_PARAMETERS roles respectively.
A logic flaw was found which allow addresses that had no limitation at some point to continue exceeding the maxDelegates. More specifically, if an account has been set to true in the canContractExceedMaxDelegates mapping and has more delegations than the ones allowed, removing it from the mapping does not decrease its delegation number to the maximum allowed. See the proof of concept below for a clearer view of the issue.
This finding was only rated as low risk due to its limited exploitation and likelihood.
Add the highlighted lines to the testDelegateOverMaxDelegatesApproved() test function in the ERC20MultiVotes.t.sol test file like shown below:
function testDelegateOverMaxDelegatesApproved() public { token.mint(address(this), 100e18); token.setMaxDelegates(2); token.setContractExceedMaxDelegates(address(this), true); token.incrementDelegation(delegate1, 50e18); token.incrementDelegation(delegate2, 1e18); token.incrementDelegation(address(this), 1e18); require(token.delegateCount(address(this)) == 3); require(token.delegateCount(address(this)) > token.maxDelegates()); require(token.userDelegatedVotes(address(this)) == 52e18); + assertTrue(token.canContractExceedMaxDelegates(address(this))); + // remove maxDelegates privilege + token.setContractExceedMaxDelegates(address(this), false); + assertFalse(token.canContractExceedMaxDelegates(address(this))); + require(token.delegateCount(address(this)) == 3); + require(token.delegateCount(address(this)) > token.maxDelegates()); + require(token.userDelegatedVotes(address(this)) == 52e18); + // see it can still increment + token.incrementDelegation(delegate2, 1e18); + require(token.userDelegatedVotes(address(this)) == 53e18); + token.incrementDelegation(address(this), 1e18); + require(token.userDelegatedVotes(address(this)) == 54e18); }
Then run it with forge test --mc ERC20MultiVotesUnitTest --silent -vvvv --mt "\btestDelegateOverMaxDelegatesApproved\b".
Observe that the user without the privilege to exceed maxDelegates (canContractExceedMaxDelegates set to false) can still have more delegations than allowed and increment delegations (if they are still to an already allowed address).
The following lines contain a casting from uint256 to uint256, which is completely unnecessary.
Similar finding from another contest: https://solodit.xyz/issues/g-06-unnecessary-casting-as-variable-is-already-of-the-same-type-code4rena-frankencoin-frankencoin-git
Consider removing the redundant casts.
The lack of explicit casting between different variable types, hinders code’s readability, making it more error-prone and hard to maintain.
The following lines contain an implicit casting from uint128 argument (_bufferCap) to a uint224 (bufferStored):
Consider explicitly casting all integer values to their expected type and use SafeCast to safely convert between different integer types as a best practice.
uint Types in EventsWhile the RateLimitedV2.sol file had uint variables of different types (uint256, uint128, uint32 and uint224), the events from the inherited interface IRateLimitedV2 were all using uint256.
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/utils/RateLimitedV2.sol#L106 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/utils/RateLimitedV2.sol#L134 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/utils/RateLimitedV2.sol#L155
Event definitions: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/utils/IRateLimitedV2.sol#L39-L52
Consider updating the events or variable types so they are consistent.
A few typos were found in some comments:
Consider fixing the typos for improved readability.
#0 - c4-pre-sort
2024-01-05T18:16:30Z
0xSorryNotSorry marked the issue as sufficient quality report
#1 - Trumpero
2024-01-31T02:02:14Z
L-01: NC L-02: low L-03: NC L-04: low L-05: low NC-01: bot (N-62) NC-02: NC NC-03: NC NC-04: NC
#2 - Trumpero
2024-01-31T11:57:29Z
#3 - c4-judge
2024-01-31T11:57:33Z
Trumpero marked the issue as grade-b