Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes after internal audit #118

Merged
merged 4 commits into from
Mar 14, 2025
Merged

Fixes after internal audit #118

merged 4 commits into from
Mar 14, 2025

Conversation

toninorair
Copy link
Contributor

@toninorair toninorair commented Mar 13, 2025 •
edited
Loading

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 13, 2025 •
edited
Loading

Changes to gas cost

Generated at commit: fdf362821f274bf6b55a698d2e0fa645dd119007, compared to commit: b742ab8e47fb9fbed103bc9e915555b1d688715d

🧾 Summary (20% most significant diffs)

Contract Method Avg (+/-) %
WrappedMTokenHarness currentIndex
excess
internalUnwrap
internalWrap
totalAccruedYield
wrapWithPermit(address,uint256,uint256,bytes)
wrapWithPermit(address,uint256,uint256,uint8,bytes32,bytes32)		 -97 ✅
+1,984 ❌
+2,849 ❌
+5,215 ❌
-146 ✅
+5,002 ❌
+5,001 ❌		 -7.41%
+40.96%
+9.91%
+13.91%
-7.23%
+9.60%
+9.70%
WrappedMToken unwrap
wrap
wrapWithPermit(address,uint256,uint256,uint8,bytes32,bytes32)		 +7,319 ❌
+6,048 ❌
+26,891 ❌		 +10.32%
+12.78%
+16.76%
Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
MockM 534,344 (0) balanceOf
currentIndex
setBalanceOf
setPrincipalBalanceOf		 571 (0)
334 (0)
24,181 (0)
24,239 (0)		 0.00%
0.00%
0.00%
0.00%		 1,342 (+447)
823 (+7)
43,405 (-216)
43,222 (-583)		 +49.94%
+0.86%
-0.50%
-1.33%		 571 (0)
334 (0)
44,141 (+12)
44,187 (+12)		 0.00%
0.00%
+0.03%
+0.03%		 2,571 (0)
2,334 (0)
44,261 (0)
44,307 (0)		 0.00%
0.00%
0.00%
0.00%		 1,852 (+1,224)
2,595 (-99)
626 (0)
202 (0)
WrappedMTokenHarness 5,431,894 (+95,962) accruedYieldOf
balanceOf
balanceWithYieldOf
claimExcess
claimFor
currentIndex
disableEarning
excess
getInternalClaimRecipientOf
internalUnwrap
internalWrap
setAccountOf(address,uint256)
setAccountOf(address,uint256,uint256,bool,bool)
setEnableMIndex
setHasEarnerDetails
setTotalEarningPrincipal
setTotalEarningSupply
setTotalNonEarningSupply
startEarningFor(address)
startEarningFor(address[])
stopEarningFor(address)
totalAccruedYield
totalEarningPrincipal
totalEarningSupply
totalNonEarningSupply
totalSupply
transfer
unwrap
wrap
wrapWithPermit(address,uint256,uint256,bytes)
wrapWithPermit(address,uint256,uint256,uint8,bytes32,bytes32)		 686 (0)
633 (0)
958 (0)
16,995 (+2,067)
5,030 (-22)
612 (0)
8,736 (0)
3,294 (-26)
633 (-22)
711 (0)
13,626 (+4,664)
5,279 (0)
8,261 (0)
5,340 (0)
5,583 (0)
2,538 (0)
2,582 (+43)
2,583 (0)
2,530 (0)
2,635 (0)
8,599 (0)
1,217 (-49)
426 (0)
403 (0)
425 (0)
512 (0)
2,957 (0)
483 (0)
482 (0)
4,802 (0)
4,268 (0)		 0.00%
0.00%
0.00%
+13.85%
-0.44%
0.00%
0.00%
-0.78%
-3.36%
0.00%
+52.04%
0.00%
0.00%
0.00%
0.00%
0.00%
+1.69%
0.00%
0.00%
0.00%
0.00%
-3.87%
0.00%
0.00%
0.00%
0.00%
0.00%
0.00%
0.00%
0.00%
0.00%		 1,441 (-38)
807 (+4)
1,955 (-53)
23,883 (+1,117)
46,674 (-280)
1,212 (-97)
14,418 (-31)
6,828 (+1,984)
1,133 (-22)
31,612 (+2,849)
42,704 (+5,215)
24,569 (-316)
44,360 (+334)
6,274 (+60)
14,690 (-236)
21,513 (+119)
21,247 (+272)
21,366 (-387)
88,912 (-6)
44,217 (+191)
62,958 (-850)
1,872 (-146)
1,770 (+7)
1,422 (-3)
1,257 (-4)
1,660 (-18)
23,232 (+509)
49,054 (+3,951)
51,215 (+4,159)
57,107 (+5,002)
56,576 (+5,001)		 -2.57%
+0.50%
-2.64%
+4.91%
-0.60%
-7.41%
-0.21%
+40.96%
-1.90%
+9.91%
+13.91%
-1.27%
+0.76%
+0.97%
-1.58%
+0.56%
+1.30%
-1.78%
-0.01%
+0.43%
-1.33%
-7.23%
+0.40%
-0.21%
-0.32%
-1.07%
+2.24%
+8.76%
+8.84%
+9.60%
+9.70%		 1,587 (0)
633 (0)
2,703 (-93)
19,949 (+1,974)
10,611 (-115)
739 (-1,047)
10,833 (0)
6,467 (+2,067)
633 (-22)
31,486 (+4,670)
46,371 (+6,230)
25,179 (0)
45,261 (0)
5,340 (0)
22,683 (0)
22,438 (0)
22,482 (+43)
22,483 (0)
92,928 (+160)
51,361 (+160)
63,721 (0)
2,158 (-160)
2,426 (0)
2,403 (0)
425 (0)
2,512 (0)
15,576 (0)
48,482 (+4,670)
48,503 (+4,670)
63,755 (+18,107)
63,225 (+18,107)		 0.00%
0.00%
-3.33%
+10.98%
-1.07%
-58.62%
0.00%
+46.98%
-3.36%
+17.41%
+15.52%
0.00%
0.00%
0.00%
0.00%
0.00%
+0.19%
0.00%
+0.17%
+0.31%
0.00%
-6.90%
0.00%
0.00%
0.00%
0.00%
0.00%
+10.66%
+10.65%
+39.67%
+40.13%		 2,558 (-93)
2,633 (0)
2,703 (-93)
47,351 (+1,974)
135,134 (+415)
2,612 (0)
23,685 (-93)
13,307 (+1,996)
2,633 (-22)
62,201 (+2,042)
64,871 (+4,830)
25,179 (0)
45,261 (0)
22,440 (0)
22,683 (0)
22,438 (0)
22,482 (+43)
22,483 (0)
92,928 (+160)
78,657 (+413)
69,175 (-93)
3,217 (-49)
2,426 (0)
2,403 (0)
2,425 (0)
2,512 (0)
93,804 (+160)
65,024 (+4,842)
82,703 (+4,670)
84,515 (-25,620)
83,985 (-25,620)		 -3.51%
0.00%
-3.33%
+4.35%
+0.31%
0.00%
-0.39%
+17.65%
-0.83%
+3.39%
+8.04%
0.00%
0.00%
0.00%
0.00%
0.00%
+0.19%
0.00%
+0.17%
+0.53%
-0.13%
-1.50%
0.00%
0.00%
0.00%
0.00%
+0.17%
+8.05%
+5.98%
-23.26%
-23.37%		 341 (0)
1,726 (+6)
7 (0)
101 (0)
107 (0)
2,865 (+11)
3 (0)
107 (0)
4 (0)
9 (0)
6 (0)
705 (-11)
458 (+11)
567 (-39)
92 (-5)
641 (+11)
443 (+11)
798 (-11)
105 (0)
3 (0)
103 (0)
127 (0)
754 (+11)
995 (+23)
1,098 (-16)
108 (0)
213 (0)
102 (0)
103 (0)
102 (0)
102 (0)
WrappedMToken 4,879,511 (+99,030) CLAIM_OVERRIDE_RECIPIENT_KEY_PREFIX
EARNERS_LIST_NAME
accruedYieldOf
balanceOf
balanceWithYieldOf
claimExcess
claimFor
currentIndex
disableIndex
earnerManager
enableEarning
excess
implementation
isEarning
isEarningEnabled
mToken
migrate()
migrate(address)
migrationAdmin
startEarningFor
stopEarningFor
symbol
totalAccruedYield
totalEarningSupply
totalNonEarningSupply
transfer
transferFrom
unwrap
wrap
wrapWithPermit(address,uint256,uint256,bytes)
wrapWithPermit(address,uint256,uint256,uint8,bytes32,bytes32)		 306 (+23)
306 (-22)
686 (0)
655 (+44)
892 (-22)
73,972 (+1,952)
30,806 (-49)
716 (0)
401 (-22)
293 (-22)
59,946 (-22)
3,316 (-26)
434 (+66)
2,585 (-22)
2,421 (+23)
293 (-22)
23,125 (-22)
17,803 (-22)
293 (-22)
47,426 (+183)
51,902 (-93)
1,452 (-22)
2,180 (-132)
447 (+67)
425 (0)
42,398 (+225)
32,767 (-70)
48,510 (+4,670)
47,717 (+4,874)
129,010 (+7,013)
187,304 (+26,891)		 +8.13%
-6.71%
0.00%
+7.20%
-2.41%
+2.71%
-0.16%
0.00%
-5.20%
-6.98%
-0.04%
-0.78%
+17.93%
-0.84%
+0.96%
-6.98%
-0.10%
-0.12%
-6.98%
+0.39%
-0.18%
-1.49%
-5.71%
+17.63%
0.00%
+0.53%
-0.21%
+10.65%
+11.38%
+5.75%
+16.76%		 306 (+23)
306 (-22)
2,922 (-88)
1,104 (+44)
2,459 (-70)
73,972 (+1,952)
38,929 (+39)
1,945 (-69)
401 (-22)
293 (-22)
64,173 (-22)
3,793 (+81)
434 (+66)
2,585 (-22)
2,421 (+23)
293 (-22)
23,125 (-22)
17,803 (-22)
293 (-22)
59,547 (+185)
58,060 (-93)
1,452 (-22)
2,578 (-132)
447 (+67)
469 (-1)
56,523 (+161)
41,299 (+48)
78,265 (+7,319)
53,361 (+6,048)
129,010 (+7,013)
187,304 (+26,891)		 +8.13%
-6.71%
-2.92%
+4.15%
-2.77%
+2.71%
+0.10%
-3.43%
-5.20%
-6.98%
-0.03%
+2.18%
+17.93%
-0.84%
+0.96%
-6.98%
-0.10%
-0.12%
-6.98%
+0.31%
-0.16%
-1.49%
-4.87%
+17.63%
-0.21%
+0.29%
+0.12%
+10.32%
+12.78%
+5.75%
+16.76%		 306 (+23)
306 (-22)
3,053 (-93)
655 (+44)
3,259 (-115)
73,972 (+1,952)
31,704 (-49)
2,165 (-93)
401 (-22)
293 (-22)
59,946 (-22)
3,329 (-26)
434 (+66)
2,585 (-22)
2,421 (+23)
293 (-22)
23,125 (-22)
17,803 (-22)
293 (-22)
59,678 (+183)
58,060 (-93)
1,452 (-22)
2,802 (-137)
447 (+67)
425 (0)
60,268 (+237)
46,405 (+160)
74,162 (+14,066)
47,717 (+4,874)
129,010 (+7,013)
187,304 (+26,891)		 +8.13%
-6.71%
-2.96%
+7.20%
-3.41%
+2.71%
-0.15%
-4.12%
-5.20%
-6.98%
-0.04%
-0.77%
+17.93%
-0.84%
+0.96%
-6.98%
-0.10%
-0.12%
-6.98%
+0.31%
-0.16%
-1.49%
-4.66%
+17.63%
0.00%
+0.39%
+0.35%
+23.41%
+11.38%
+5.75%
+16.76%		 306 (+23)
306 (-22)
7,553 (-93)
2,655 (+44)
7,759 (-115)
73,972 (+1,952)
54,279 (+216)
4,165 (-93)
401 (-22)
293 (-22)
73,034 (-22)
13,031 (+1,974)
434 (+66)
2,585 (-22)
2,421 (+23)
293 (-22)
23,125 (-22)
17,803 (-22)
293 (-22)
70,878 (+183)
64,218 (-93)
1,452 (-22)
2,802 (-137)
447 (+67)
2,425 (0)
76,785 (+237)
48,405 (+160)
112,705 (+9,828)
140,032 (+26,960)
129,010 (+7,013)
187,304 (+26,891)		 +8.13%
-6.71%
-1.22%
+1.69%
-1.46%
+2.71%
+0.40%
-2.18%
-5.20%
-6.98%
-0.03%
+17.85%
+17.93%
-0.84%
+0.96%
-6.98%
-0.10%
-0.12%
-6.98%
+0.26%
-0.14%
-1.49%
-4.66%
+17.63%
0.00%
+0.31%
+0.33%
+9.55%
+23.84%
+5.75%
+16.76%		 1 (0)
1 (0)
501 (0)
1,863 (0)
125 (0)
1 (0)
3 (0)
15 (0)
6 (0)
4 (0)
5 (0)
256 (+1)
2 (0)
2 (0)
2 (0)
4 (0)
1 (0)
1 (0)
4 (0)
126 (+2)
2 (0)
1 (0)
50 (+1)
46 (+1)
45 (+1)
324 (0)
216 (0)
13 (+4)
220 (+4)
1 (0)
1 (0)
MockRegistrar 180,227 (0) get
set		 405 (0)
44,020 (0)		 0.00%
0.00%		 2,161 (-11)
44,315 (-5)		 -0.51%
-0.01%		 2,405 (0)
44,392 (0)		 0.00%
0.00%		 2,405 (0)
44,392 (0)		 0.00%
0.00%		 189 (-9)
63 (-5)
ListOfEarnersToMigrate 0 (0) getEarners 2,617 (0) 0.00% 74,688 (+312) +0.42% 81,552 (0) 0.00% 81,552 (0) 0.00% 23 (+1)
WrappedMTokenMigratorV1 1,474,194 (0) fallback 11,150 (0) 0.00% 228,180 (+933) +0.41% 248,690 (0) 0.00% 249,807 (0) 0.00% 23 (+1)
Proxy 104,329 (0) fallback 432 (0) 0.00% 21,970 (+18) +0.08% 2,712 (0) 0.00% 142,479 (+1,350) +0.96% 15,401 (+105)
MockEarnerManager 194,427 (0) setEarnerDetails 22,790 (0) 0.00% 44,714 (-7) -0.02% 44,702 (0) 0.00% 44,966 (0) 0.00% 211 (-5)
EarnerManager 1,377,221 (0) getEarnerDetails 9,453 (0) 0.00% 9,471 (-1) -0.01% 9,453 (0) 0.00% 11,741 (0) 0.00% 122 (+2)

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 13, 2025 •
edited
Loading

LCOV of commit 0a2ed8c during Forge Coverage #616

Summary coverage rate:
  lines......: 99.2% (366 of 369 lines)
  functions..: 100.0% (74 of 74 functions)
  branches...: 95.8% (23 of 24 branches)

Files changed coverage rate:
                                 |Lines       |Functions  |Branches    
  Filename                       |Rate     Num|Rate    Num|Rate     Num
  =====================================================================
  src/WrappedMToken.sol          |20.7%    241| 0.0%    50|    -      0

@toninorair toninorair changed the title Add suggested optimizations Fixes after internal audit Mar 13, 2025
@toninorair toninorair changed the title Fixes after internal audit Fixes after Internal audit Mar 13, 2025
@toninorair toninorair changed the title Fixes after Internal audit Fixes after internal audit Mar 13, 2025
@toninorair toninorair marked this pull request as ready for review March 13, 2025 20:19
@toninorair toninorair requested a review from PierrickGT March 13, 2025 20:22
PierrickGT
PierrickGT requested changes Mar 13, 2025
View reviewed changes
src/WrappedMToken.sol
return
earmarked_ == 0 ? int248(uint248(balance_)) : int248(uint248(balance_)) - int248(uint248(earmarked_));
// Decreases claimable excess by the `roundingError` for extra level of safety and solvency.
return int240(balance_) - int240(earmarked_) - roundingError;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be + roundingError since it can be a positive or negative number.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it is intentionally int vs uint.
Though it is positive if protocol occurred 'loss', so it needs to be subtracted from excess,
and error is negative when protocol has a surplus, so -(-) cancels out into + and abs of rounding error will be added to excess

@toninorair toninorair merged commit 110b8f6 into feat/blackthorn-audit-fixes Mar 14, 2025
4 checks passed
@toninorair toninorair deleted the feat/internal-audit-fixes branch March 14, 2025 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PierrickGT PierrickGT PierrickGT approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@toninorair @PierrickGT