Platform: Code4rena
Start Date: 25/10/2022
Pot Size: $50,000 USDC
Total HM: 18
Participants: 127
Period: 5 days
Judge: 0xean
Total Solo HM: 9
Id: 175
League: ETH
Rank: 66/127
Findings: 2
Award: $55.74
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: 0x1f8b
Also found by: 0xNazgul, 0xSmartContract, Aymen0909, B2, Bnke0x0, Deivitto, Diana, Dinesh11G, ElKu, JC, Josiah, Rahoz, RaymondFam, ReyAdmirado, Rolezn, Waze, __141345__, adriro, aphak5010, brgltd, c3phas, c7e7eff, carlitox477, cducrest, ch0bu, chrisdior4, cryptonue, cryptostellar5, cylzxje, d3e4, delfin454000, enckrish, evmwanderer, fatherOfBlocks, gogo, hansfriese, horsefacts, immeas, leosathya, lukris02, neumo, oyc_109, pedr02b2, rbserver, robee, rotcivegaf, rvierdiiev, sakshamguruji, shark, simon135, tnevler, trustindistrust, wagmi
36.7345 USDC - $36.73
| Risk | Title | Instances |
|---|---|---|
| L-00 | Use 2-Step-Process for assigning roles | - |
| L-01 | Missing zero address check can lead to loss of voting power | 2 |
| N-00 | Lines too long | 4 |
| N-01 | Remove TODO comments | 1 |
| N-02 | Remove code that is commented out | 2 |
| N-03 | Unlocked pragma | 8 |
| N-04 | Make public functions that are not called internally external | 65 |
| N-05 | Typos | 2 |
| N-06 | Comment not according to logic | 1 |
A 2-Step-Process should be used for assigning roles that are critical to the operation of the contract.
E.g. if a wrong gov is set in the Market contract, the contract is lost.
There several roles that a 2-Step-Process should be implemented for:
Market.sol
Fed.sol
BorrowController.sol
The 2-Step-Process can be implemented like setPendingOperator(address newOperator_) and claimOperator() in the Oracle and DBR contracts where a 2-Step-Process is already used.
The GovTokenEscrow and INVEscrow can both hold ERC20 tokens with a delegate function.
It is left to the implementation of the ERC20 token to check if the address specified is the zero address.
However it should not be relied on this and the escrow contract itself should check for zero address.
Fix:
function delegate(address delegatee) public { require(msg.sender == beneficiary); + require(delegatee != address(0)); token.delegate(delegatee); }
The maximum line length should be 164 characters.
That's because GiHub will not display lines longer than that on the screen without scrolling.
There are 4 instances of this.
https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/escrows/GovTokenEscrow.sol#L16
TODO comments should be removed from production code.
There is 1 instance of this.
https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/escrows/INVEscrow.sol#L35
Any code that is not needed should be removed from the production codebase.
There are 2 instances of this.
https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/escrows/GovTokenEscrow.sol#L56-L60
All the contracts use ^0.8.13 as the Solidity version.
It is considered best practice to use a locked Solidity version, thereby only allowing compilation with a specific version like 0.8.13.
public functions that are not called internally externalIt is best practice to set the function visibility for functions that are not called by a contract internally to external.
There are 65 instances of this.
https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/escrows/GovTokenEscrow.sol#L30
The new replen should be called something like The new replenishment price in basis pointsamount od should say amount ofThe comment states that the voting power of the underlying xINV is delegated.
However in the GovTokenEscrow contract, there is no xINV voting power delegated.
#0 - c4-judge
2022-11-07T19:50:41Z
0xean marked the issue as grade-b
🌟 Selected for report: pfapostol
Also found by: 0x1f8b, 0xRoxas, 0xSmartContract, Amithuddar, Aymen0909, B2, Bnke0x0, Chandr, CloudX, Deivitto, Diana, Dinesh11G, ElKu, HardlyCodeMan, JC, JrNet, KoKo, Mathieu, Ozy42, Rahoz, RaymondFam, ReyAdmirado, Rolezn, Shinchan, __141345__, adriro, ajtra, aphak5010, ballx, c3phas, carlitox477, ch0bu, chaduke, cryptostellar5, djxploit, durianSausage, enckrish, exolorkistis, fatherOfBlocks, gogo, horsefacts, kaden, karanctf, leosathya, martin, mcwildy, oyc_109, ret2basic, robee, sakman, sakshamguruji, shark, skyle, tnevler
19.0072 USDC - $19.01
The Gas savings are calculated using the forge test --gas-report command.
The same tests were used that are provided in the contest repository.
| Issue | Title | Instances | Estimated Gas Savings (Deployments) | Estimated Gas Savings (Method calls) |
|---|---|---|---|---|
| G-00 | Use <10001 instead of <=10000 | 1 | 200 | 3 |
| G-01 | Use functions instead of modifiers | 4 modifiers | 101306 | - |
| G-02 | X = X + Y costs less Gas than X += Y | 7 | 10214 | 237 |
| G-03 | dola variable can be constant | 1 | 23524 | 18 |
| G-04 | Load feeds[token] into memory instead of accessing instance variable | 2 functions | 13613 | 992 |
<10001 instead of <=10000Deployment Gas saved: 200
Method call Gas saved: 3
Fix:
require(_liquidationFactorBps > 0 && _liquidationFactorBps < 10001, "Invalid liquidation factor");
Modifiers make code more elegant and readable but cost more Gas than functions.
Arguably, the additional Gas cost outweighs the readability benefit.
There are 4 modifiers defined in the codebase:
| Modifier | Deployment Gas saved |
|---|---|
BorrowController.sol: onlyOperator | 7006 |
DBR.sol: onlyOperator | 23627 |
Oracle.sol: onlyOperator | 10006 |
Market.sol: onlyGov | 60667 |
Example of changing modifier onlyGov in market to a function:
- modifier onlyGov { + function onlyGov() internal view { require(msg.sender == gov, "Only gov can call this function"); - _; }
The function can then be used like this:
- function setLiquidationFactorBps(uint _liquidationFactorBps) public onlyGov { + function setLiquidationFactorBps(uint _liquidationFactorBps) public { + onlyGov(); require(_liquidationFactorBps > 0 && _liquidationFactorBps <= 10000, "Invalid liquidation factor"); liquidationFactorBps = _liquidationFactorBps; }
X = X + Y costs less Gas than X += YThe same is true for X -= Y.
Instances of this are included here as well.
This does usually not work with structs or mappings.
Deployment Gas saved (all instances): 10214
Method call Gas saved (all instances): 237
There are 7 instances of this.
https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/DBR.sol#L289
Fix:
- totalDueTokensAccrued += accrued; + totalDueTokensAccrued = totalDueTokensAccrued + accrued;
dola variable can be constantThe dola variable in Market.sol can be declared constant instead of immutable.
Deployment Gas saved: 23524
Method call Gas saved: 18 for every access
feeds[token] into memory instead of accessing instance variableThe Oracle.viewPrice() and Oracle.getPrice() functions use feeds[token] multiple times.
Therefore feeds[token] should be loaded into memory and not be accessed from storage each time.
Deployment Gas saved (both functions together): 13613
Method call Gas saved (both functions together): 992
Fix:
@@ -77,13 +77,14 @@ contract Oracle { */ function viewPrice(address token, uint collateralFactorBps) external view returns (uint) { if(fixedPrices[token] > 0) return fixedPrices[token]; - if(feeds[token].feed != IChainlinkFeed(address(0))) { + FeedData memory feedData = feeds[token]; + if(feedData.feed != IChainlinkFeed(address(0))) { // get price from feed - uint price = feeds[token].feed.latestAnswer(); + uint price = feedData.feed.latestAnswer(); require(price > 0, "Invalid feed price"); // normalize price - uint8 feedDecimals = feeds[token].feed.decimals(); - uint8 tokenDecimals = feeds[token].tokenDecimals; + uint8 feedDecimals = feedData.feed.decimals(); + uint8 tokenDecimals = feedData.tokenDecimals; uint8 decimals = 36 - feedDecimals - tokenDecimals; uint normalizedPrice = price * (10 ** decimals); uint day = block.timestamp / 1 days; @@ -111,13 +112,14 @@ contract Oracle { */ function getPrice(address token, uint collateralFactorBps) external returns (uint) { if(fixedPrices[token] > 0) return fixedPrices[token]; - if(feeds[token].feed != IChainlinkFeed(address(0))) { + FeedData memory feedData = feeds[token]; + if(feedData.feed != IChainlinkFeed(address(0))) { // get price from feed - uint price = feeds[token].feed.latestAnswer(); + uint price = feedData.feed.latestAnswer(); require(price > 0, "Invalid feed price"); // normalize price - uint8 feedDecimals = feeds[token].feed.decimals(); - uint8 tokenDecimals = feeds[token].tokenDecimals; + uint8 feedDecimals = feedData.feed.decimals(); + uint8 tokenDecimals = feedData.tokenDecimals; uint8 decimals = 36 - feedDecimals - tokenDecimals; uint normalizedPrice = price * (10 ** decimals); // potentially store price as today's low
#0 - c4-judge
2022-11-05T23:40:44Z
0xean marked the issue as grade-b