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.

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

fix(contracts): add rebasing compatibility for HypERC4626 #4524

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

aroralanuk
Copy link
Contributor

Description

  • Added overrides for transferFrom, totalSupply to reflect the internal share based accounting for the 4626 mirror asset

Drive-by changes

  • Overridden _transfer to update the Transfer event to display the asset being transfers as amount not the internal shares.

Related issues

Backward compatibility

Yes

Testing

Fuzz testing

Copy link

changeset-bot bot commented Sep 19, 2024

⚠️ No Changeset found

Latest commit: 1406efb

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.08%. Comparing base (9f5a17b) to head (1406efb).
Report is 17 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (9f5a17b) and HEAD (1406efb). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (9f5a17b) HEAD (1406efb)
2 1
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4524      +/-   ##
==========================================
- Coverage   82.33%   74.08%   -8.25%     
==========================================
  Files         100      100              
  Lines        1421     1428       +7     
  Branches      180      180              
==========================================
- Hits         1170     1058     -112     
- Misses        251      349      +98     
- Partials        0       21      +21     
Components Coverage Δ
core 84.61% <ø> (-12.31%) ⬇️
hooks 75.71% <ø> (-10.96%) ⬇️
isms 79.20% <ø> (-14.61%) ⬇️
token 89.37% <100.00%> (-4.09%) ⬇️
middlewares 77.39% <ø> (-9.14%) ⬇️

@@ -172,6 +177,108 @@
);
}

function testTransferFrom() public {

Check notice

Code scanning / Olympix Integrated Security

Reentrant functions which emit events after making an external call may lead to out-of-order events. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/reentrancy-events

Reentrant functions which emit events after making an external call may lead to out-of-order events. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/reentrancy-events

event Transfer(address indexed from, address indexed to, uint256 value);

function testTransferEvent() public {

Check notice

Code scanning / Olympix Integrated Security

Reentrant functions which emit events after making an external call may lead to out-of-order events. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/reentrancy-events

Reentrant functions which emit events after making an external call may lead to out-of-order events. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/reentrancy-events
);
}

function testTotalShares() public {

Check notice

Code scanning / Olympix Integrated Security

Reentrant functions which emit events after making an external call may lead to out-of-order events. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/reentrancy-events

Reentrant functions which emit events after making an external call may lead to out-of-order events. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/reentrancy-events
);
}

function testShareBalanceOf() public {

Check notice

Code scanning / Olympix Integrated Security

Reentrant functions which emit events after making an external call may lead to out-of-order events. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/reentrancy-events

Reentrant functions which emit events after making an external call may lead to out-of-order events. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/reentrancy-events
@@ -385,6 +492,32 @@
);
}

function testTotalSupply() public {

Check notice

Code scanning / Olympix Integrated Security

Reentrant functions which emit events after making an external call may lead to out-of-order events. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/reentrancy-events

Reentrant functions which emit events after making an external call may lead to out-of-order events. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/reentrancy-events
Copy link
Collaborator

Choose a reason for hiding this comment

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

annoying that this is showing up here

Comment on lines 44 to 45
address owner = _msgSender();
_transfer(owner, to, assetsToShares(amount));
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: unnecessary intermediate variable

Suggested change
address owner = _msgSender();
_transfer(owner, to, assetsToShares(amount));
_transfer(_msgSender(), to, assetsToShares(amount));

Comment on lines 65 to 67
function totalSupply() public view virtual override returns (uint256) {
return sharesToAssets(super.totalSupply());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
function totalSupply() public view virtual override returns (uint256) {
return sharesToAssets(super.totalSupply());
}
function totalSupply() public view virtual override returns (uint256) {
return sharesToAssets(totalShares());
}

Comment on lines 71 to 75
function balanceOf(
address account
) public view virtual override returns (uint256) {
return sharesToAssets(super.balanceOf(account));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
function balanceOf(
address account
) public view virtual override returns (uint256) {
return sharesToAssets(super.balanceOf(account));
}
function balanceOf(
address account
) public view virtual override returns (uint256) {
return sharesToAssets(shareBalanceOf(account));
}

function sharesToAssets(uint256 _shares) public view returns (uint256) {
return _shares.mulDiv(exchangeRate, PRECISION);
) internal virtual override {
super._transfer(sender, recipient, amount);
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this not already emit an event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does but the event will shares amount as transferAmount in the event which is not desirable

@@ -385,6 +492,32 @@
);
}

function testTotalSupply() public {
Copy link
Collaborator

Choose a reason for hiding this comment

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

annoying that this is showing up here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

2 participants