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

fix: correct handling of non-18 decimal reward tokens #35

Merged
merged 6 commits into from
Sep 4, 2024
Merged

fix: correct handling of non-18 decimal reward tokens #35

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0cd0724
Feat/mapping comment update (#18)
stevenvaleri Sep 2, 2024
d093359
fix: ensure reward fees are only transferred if the reward token is s…
jparklev Sep 2, 2024
ce62750
fix: correct handling of non-18 decimal reward tokens
jparklev Sep 2, 2024
2de22e7
fix: revert rewards collection change from different branch
jparklev Sep 3, 2024
8fbebe0
fix: revert rewards collection test change from different branch
jparklev Sep 3, 2024
f77d12d
fix: correct reversion commit
jparklev Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 26 additions & 14 deletions contracts/PointTokenVault.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ contract PointTokenVault is UUPSUpgradeable, AccessControlUpgradeable, Multicall
// Merkle root distribution.
bytes32 public currRoot;
bytes32 public prevRoot;
mapping(address => mapping(bytes32 => uint256)) public claimedPTokens; // user => pointsId => claimed
mapping(address => mapping(bytes32 => uint256)) public claimedRedemptionRights; // user => pointsId => claimed
mapping(address => mapping(bytes32 => uint256)) public claimedPTokens; // user => pointsId => PTokens claimed
mapping(address => mapping(bytes32 => uint256)) public claimedRedemptionRights; // user => pointsId => Rewards redeemed

mapping(bytes32 => PToken) public pTokens; // pointsId => pTokens

Expand Down Expand Up @@ -89,7 +89,7 @@ contract PointTokenVault is UUPSUpgradeable, AccessControlUpgradeable, Multicall

error ProofInvalidOrExpired();
error ClaimTooLarge();
error RewardsNotReleased();
error RewardsNotLive();
error CantConvertMerkleRedemption();
error PTokenAlreadyDeployed();
error DepositExceedsCap();
Expand Down Expand Up @@ -177,7 +177,7 @@ contract PointTokenVault is UUPSUpgradeable, AccessControlUpgradeable, Multicall
(params.rewardToken, params.rewardsPerPToken, params.isMerkleBased);

if (address(rewardToken) == address(0)) {
revert RewardsNotReleased();
revert RewardsNotLive();
}

if (isMerkleBased) {
Expand All @@ -188,7 +188,9 @@ contract PointTokenVault is UUPSUpgradeable, AccessControlUpgradeable, Multicall
_verifyClaimAndUpdateClaimed(_claim, claimHash, msg.sender, claimedRedemptionRights);
}

uint256 pTokensToBurn = FixedPointMathLib.divWadUp(amountToClaim, rewardsPerPToken);
uint256 scalingFactor = 10 ** (18 - rewardToken.decimals());
uint256 pTokensToBurn = FixedPointMathLib.divWadUp(amountToClaim * scalingFactor, rewardsPerPToken);

pTokens[pointsId].burn(msg.sender, pTokensToBurn);

uint256 claimed = claimedPTokens[msg.sender][pointsId];
Expand All @@ -205,12 +207,17 @@ contract PointTokenVault is UUPSUpgradeable, AccessControlUpgradeable, Multicall
rewardsToTransfer = amountToClaim;
feelesslyRedeemedPTokens[msg.sender][pointsId] += pTokensToBurn;
} else {
// If some or all of the pTokens need to be charged a fee.
uint256 redeemableWithFee = pTokensToBurn - feelesslyRedeemable;
// fee = amount of pTokens that are not feeless * rewardsPerPToken * redemptionFee
fee = FixedPointMathLib.mulWadUp(
FixedPointMathLib.mulWadUp(redeemableWithFee, rewardsPerPToken), redemptionFee
);
// Calculate the fee. Scope avoids stack too deep errors.
{
// If some or all of the pTokens need to be charged a fee.
uint256 redeemableWithFee = pTokensToBurn - feelesslyRedeemable;
// fee = amount of pTokens that are not feeless * rewardsPerPToken * redemptionFee
fee = FixedPointMathLib.mulWadUp(
FixedPointMathLib.mulWadUp(redeemableWithFee, rewardsPerPToken), redemptionFee
);

fee = fee / scalingFactor; // Downscale to reward token decimals.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just calling out that we are rounding down here. Makes sense, and is nice to round in favor of the user here.

}

rewardTokenFeeAcc[pointsId] += fee;
rewardsToTransfer = amountToClaim - fee;
Expand All @@ -232,7 +239,7 @@ contract PointTokenVault is UUPSUpgradeable, AccessControlUpgradeable, Multicall
(params.rewardToken, params.rewardsPerPToken, params.isMerkleBased);

if (address(rewardToken) == address(0)) {
revert RewardsNotReleased();
revert RewardsNotLive();
}

if (isMerkleBased) {
Expand All @@ -241,7 +248,8 @@ contract PointTokenVault is UUPSUpgradeable, AccessControlUpgradeable, Multicall

rewardToken.safeTransferFrom(msg.sender, address(this), _amountToConvert);

uint256 pTokensToMint = FixedPointMathLib.divWadDown(_amountToConvert, rewardsPerPToken); // Round down for mint.
uint256 scalingFactor = 10 ** (18 - rewardToken.decimals());
uint256 pTokensToMint = FixedPointMathLib.divWadDown(_amountToConvert * scalingFactor, rewardsPerPToken);

// Dust guard.
if (pTokensToMint == 0) {
Expand Down Expand Up @@ -349,8 +357,12 @@ contract PointTokenVault is UUPSUpgradeable, AccessControlUpgradeable, Multicall
}

if (rewardTokenFee > 0) {
ERC20 rewardToken = redemptions[_pointsId].rewardToken;
if (address(rewardToken) == address(0)) {
revert RewardsNotLive();
}
// There will only be a positive rewardTokenFee if there are reward tokens in this contract available for transfer.
redemptions[_pointsId].rewardToken.safeTransfer(feeCollector, rewardTokenFee);
rewardToken.safeTransfer(feeCollector, rewardTokenFee);
rewardTokenFeeAcc[_pointsId] = 0;
}

Expand Down
72 changes: 64 additions & 8 deletions contracts/test/PointTokenVault.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,13 @@ import {Test, console} from "forge-std/Test.sol";
import {PointTokenVault} from "../PointTokenVault.sol";
import {PToken} from "../PToken.sol";

import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
import {ERC1967Utils} from "openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Utils.sol";
import {Pausable} from "@openzeppelin/contracts/utils/Pausable.sol";
import {IAccessControl} from "@openzeppelin/contracts/access/IAccessControl.sol";

import {MockERC20} from "solmate/test/utils/mocks/MockERC20.sol";
import {MockERC20, ERC20} from "solmate/test/utils/mocks/MockERC20.sol";

import {LibString} from "solady/utils/LibString.sol";

import {OwnableUpgradeable} from "openzeppelin-contracts-upgradeable/contracts/access/OwnableUpgradeable.sol";

import {PointTokenVaultScripts} from "../script/PointTokenVault.s.sol";

contract PointTokenVaultTest is Test {
Expand Down Expand Up @@ -409,6 +405,31 @@ contract PointTokenVaultTest is Test {
assertEq(pointTokenVault.pTokens(eigenPointsId).balanceOf(vitalik), 1e18 - 1);
}

function test_RedeemRewardsWith6DecimalToken() public {
// Setup a mock 6-decimal token (like USDC)
MockERC20 usdcReward = new MockERC20("USDC Reward", "USDC", 6);

// Mint 1,000,000 USDC to the vault
usdcReward.mint(address(pointTokenVault), 1_000_000 * 1e6);

// Set redemption parameters (1 pToken = 1 USDC)
vm.prank(operator);
pointTokenVault.setRedemption(eigenPointsId, usdcReward, 1e18, false);

// Mint 1 pToken to vitalik
vm.startPrank(address(pointTokenVault));
pointTokenVault.pTokens(eigenPointsId).mint(vitalik, 1e18);
vm.stopPrank();

// Vitalik redeems 1 pToken for 1 USDC
vm.prank(vitalik);
pointTokenVault.redeemRewards(PointTokenVault.Claim(eigenPointsId, 1e6, 1e6, new bytes32[](0)), vitalik);

// Check balances
assertEq(usdcReward.balanceOf(vitalik), 1e6, "Vitalik should receive 1 USDC");
assertEq(pointTokenVault.pTokens(eigenPointsId).balanceOf(vitalik), 0, "Vitalik should have 0 pTokens left");
}

event RewardsClaimed(
address indexed owner, address indexed receiver, bytes32 indexed pointsId, uint256 amount, uint256 tax
);
Expand Down Expand Up @@ -519,7 +540,7 @@ contract PointTokenVaultTest is Test {

event RewardsConverted(address indexed owner, address indexed receiver, bytes32 indexed pointsId, uint256 amount);

function test_MintPTokensForRewards() public {
function test_ConvertRewardsToPTokens() public {
bytes32 root = 0x4e40a10ce33f33a4786960a8bb843fe0e170b651acd83da27abc97176c4bed3c;

bytes32[] memory proof = new bytes32[](1);
Expand All @@ -535,9 +556,9 @@ contract PointTokenVaultTest is Test {

// Cannot redeem pTokens or convert rewards before redemption data is set
bytes32[] memory empty = new bytes32[](0);
vm.expectRevert(PointTokenVault.RewardsNotReleased.selector);
vm.expectRevert(PointTokenVault.RewardsNotLive.selector);
pointTokenVault.redeemRewards(PointTokenVault.Claim(eigenPointsId, 2e18, 2e18, empty), vitalik);
vm.expectRevert(PointTokenVault.RewardsNotReleased.selector);
vm.expectRevert(PointTokenVault.RewardsNotLive.selector);
pointTokenVault.convertRewardsToPTokens(vitalik, eigenPointsId, 1e18);

vm.prank(operator);
Expand Down Expand Up @@ -568,6 +589,30 @@ contract PointTokenVaultTest is Test {
assertEq(pointTokenVault.pTokens(eigenPointsId).balanceOf(vitalik), 0);
}

function test_ConvertRewardsToPTokensWith6DecimalToken() public {
// Setup a mock 6-decimal token (like USDC)
MockERC20 usdcReward = new MockERC20("USDC Reward", "USDC", 6);

// Mint 1,000,000 USDC to vitalik
usdcReward.mint(vitalik, 1_000_000 * 1e6);

// Set redemption parameters (1 pToken = 1 USDC)
vm.prank(operator);
pointTokenVault.setRedemption(eigenPointsId, usdcReward, 1e18, false);

// Approve USDC spend
vm.prank(vitalik);
usdcReward.approve(address(pointTokenVault), type(uint256).max);

// Vitalik converts 1 USDC to 1 pToken
vm.prank(vitalik);
pointTokenVault.convertRewardsToPTokens(vitalik, eigenPointsId, 1e6);

// Check balances
assertEq(usdcReward.balanceOf(vitalik), 999_999 * 1e6, "Vitalik should have 999,999 USDC left");
assertEq(pointTokenVault.pTokens(eigenPointsId).balanceOf(vitalik), 1e18, "Vitalik should receive 1 pToken");
}

event FeeCollectorSet(address feeCollector);

function test_setFeeCollector() public {
Expand Down Expand Up @@ -675,6 +720,17 @@ contract PointTokenVaultTest is Test {
vm.prank(toly);
pointTokenVault.redeemRewards(PointTokenVault.Claim(eigenPointsId, 1.8e18, 1.8e18, empty), toly);

// Unset redemption
vm.prank(operator);
pointTokenVault.setRedemption(eigenPointsId, ERC20(address(0)), 0, false);

vm.expectRevert(PointTokenVault.RewardsNotLive.selector);
pointTokenVault.collectFees(eigenPointsId);

// Set redemption again
vm.prank(operator);
pointTokenVault.setRedemption(eigenPointsId, rewardToken, 2e18, false);

// Collect fees
vm.expectEmit(true, true, true, true);
emit FeesCollected(eigenPointsId, pointTokenVault.feeCollector(), 0.1e18, 0.09e18);
Expand Down