Fix audit issue 6.6 rebasing balance transfers #55
Conversation
|
This pull request has been linked to Shortcut Story #319: ChainSecurity Audit Findings Review. |
src/ynLSD.sol
Outdated
|
|
||
| emit Deposit(sender, receiver, amount, shares); | ||
| } | ||
| if (balance < previousBalance + amount) { |
There was a problem hiding this comment.
why do this and not just
balance - previousBalance
This if clause seems unnecessary
There was a problem hiding this comment.
made an update here: 2e62297
src/ynLSD.sol
Outdated
| } | ||
| if (balance < previousBalance + amount) { | ||
| uint256 difference = previousBalance + amount - balance; | ||
| assetAmountInETH = convertToETH(asset, amount - difference); |
There was a problem hiding this comment.
i'd pull the convertToETH out of this function
looks like it does a bit too much,
_transferAsset would return in that case how much token was transferred in actuality and then do the convertToETH in the deposit function
src/ynLSD.sol
Outdated
| */ | ||
| function _convertToShares(uint256 ethAmount, Math.Rounding rounding) internal view returns (uint256) { | ||
| function _convertToShares(uint256 ethAmount, uint256 preTotalAssets, Math.Rounding rounding) internal view returns (uint256) { |
There was a problem hiding this comment.
instead of preTotalAssets, let's spell name fully
previousTotalAssets or totalAssetsBefore
src/ynLSD.sol
Outdated
| */ | ||
| function convertToETH(IERC20 asset, uint amount) public view returns (uint256) { | ||
| uint256 assetPriceInETH = oracle.getLatestPrice(address(asset)); | ||
| uint8 assetDecimals = IERC20Metadata(address(asset)).decimals(); | ||
| return assetDecimals < 18 || assetDecimals > 18 | ||
| return assetDecimals != 18 |
src/ynLSD.sol
Outdated
| @@ -439,7 +462,7 @@ contract ynLSD is IynLSD, ynBase, ReentrancyGuardUpgradeable, IynLSDEvents { | |||
| } | |||
|
|
|||
| IERC20(asset).safeTransfer(msg.sender, amount); | |||
| emit AssetRetrieved(asset, amount, nodeId, msg.sender); | |||
| emit AssetRetrieved(asset, IERC20(asset).balanceOf(msg.sender), nodeId, msg.sender); | |||
There was a problem hiding this comment.
This is not correct, there may be tokens before in the sender, it's not safe to do this
This needs to be fixed with balanceOf before and after
There was a problem hiding this comment.
i'd just emit an event for amount here, since that's what was requested, and use the balance calculation on the other side
another event will get emitted there
That one is actually important to check, so we should do this in LSDStakingNode.sol around line 92:
uint256 balanceBefore = asset.balanceOf(address(this));
ynLSD.retrieveAsset(nodeId, assets[i], amount);
uint256 balanceAfter = asset.balanceOf(address(this));
uint256 retrievedAmount = balanceAfter - balanceBefore;
There was a problem hiding this comment.
I updated all the comments currently open in this commit: 2e62297
xhad
force-pushed
the
chad/sc-319/audit-6-6-balance-transfers
branch
from
f2f033c to
2e62297
Compare
danoctavian
force-pushed
the
chad/sc-319/audit-6-6-balance-transfers
branch
2 times, most recently
from
65f0fdb to
ccd9c0a
Compare
Fix formatting
xhad
force-pushed
the
chad/sc-319/audit-6-6-balance-transfers
branch
from
ccd9c0a to
b433305
Compare
6.6 Incorrect Balance Transfers With Rebasing Tokens
Problem
The
ynLSD.depositfunction was minting shares based on the amount that was provided by the depositor, prior to callingsafeTransferFrom. It's possible that rebasing tokens will return less than the amount that was provided, so there was a potential deficit in the share accounting after the user's ynLSD tokens were minted.This PR creates a private function that handles
safeTransferFromand checks if the returned balance is less than the input amount. It then returns the amount converted to ETH.