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: separate deposit cap tracking from contract balance changes #38

Merged
merged 3 commits into from
Sep 4, 2024
Merged

fix: separate deposit cap tracking from contract balance changes #38

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0cd0724
Feat/mapping comment update (#18)
stevenvaleri Sep 2, 2024
62d3706
fix: separate deposit cap tracking from contract balance changes
jparklev Sep 2, 2024
daad4da
chore: move interaction below effects for deposit
jparklev Sep 3, 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
10 changes: 7 additions & 3 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 All @@ -43,6 +43,8 @@ contract PointTokenVault is UUPSUpgradeable, AccessControlUpgradeable, Multicall

mapping(address => mapping(address => bool)) public trustedClaimers; // owner => delegate => trustedClaimers

mapping(address => uint256) public totalDeposited; // token => total deposited amount

// Fees
uint256 public mintFee;
uint256 public redemptionFee;
Expand Down Expand Up @@ -115,20 +117,22 @@ contract PointTokenVault is UUPSUpgradeable, AccessControlUpgradeable, Multicall
uint256 cap = caps[address(_token)];

if (cap != type(uint256).max) {
if (_amount + _token.balanceOf(address(this)) > cap) {
if (totalDeposited[address(_token)] + _amount > cap) {
revert DepositExceedsCap();
}
}

_token.safeTransferFrom(msg.sender, address(this), _amount);

balances[_receiver][_token] += _amount;
totalDeposited[address(_token)] += _amount;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably set the balance and totalDeposited before the external call to avoid a re-entrancy pattern.

This is just to be extremely conservative. Given we will only ever be giving tokens a cap that we've reviewed an know do not have re-entrancy (but maybe they are upgradeable), and the impact of this re-entrancy here is just evading our cap, meant to limit exposure, not loss of assets and no negative impact to other users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 daad4da


emit Deposit(msg.sender, _receiver, address(_token), _amount);
}

function withdraw(ERC20 _token, uint256 _amount, address _receiver) public {
balances[msg.sender][_token] -= _amount;
totalDeposited[address(_token)] -= _amount;

_token.safeTransfer(_receiver, _amount);

Expand Down
41 changes: 41 additions & 0 deletions contracts/test/PointTokenVault.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,47 @@ contract PointTokenVaultTest is Test {
assertEq(pointTokenVault.balances(vitalik, newMockToken), 2e18); // Total 2 tokens deposited
}

function test_DepositCapRewardSameAsDeposit() public {
// Set up an 18 decimal token as both deposit and reward token
MockERC20 token = new MockERC20("Example Token", "EX", 18);

vm.startPrank(operator);
// Set deposit cap for token to 5000
pointTokenVault.setCap(address(token), 5000e18);

// Set token as reward token with 1:1 ratio
pointTokenVault.setRedemption(eigenPointsId, token, 1e18, false);
vm.stopPrank();

// Mint tokens to users
token.mint(vitalik, 5000e18);
token.mint(toly, 2000e18);

// Vitalik deposits 4000 tokens
vm.startPrank(vitalik);
token.approve(address(pointTokenVault), 4000e18);
pointTokenVault.deposit(token, 4000e18, vitalik);
vm.stopPrank();

// Toly converts 2000 tokens to pTokens
vm.startPrank(toly);
token.approve(address(pointTokenVault), 2000e18);
pointTokenVault.convertRewardsToPTokens(toly, eigenPointsId, 2000e18);
vm.stopPrank();

// Assert current token balance in vault
assertEq(token.balanceOf(address(pointTokenVault)), 6000e18);

// Try to deposit 1000 tokens, which should succeed
vm.startPrank(vitalik);
token.approve(address(pointTokenVault), 1000e18);
pointTokenVault.deposit(token, 1000e18, vitalik);
vm.stopPrank();

// Assert that 5000 tokens have been deposited
assertEq(pointTokenVault.balances(vitalik, token), 5000e18);
}

function test_DeployPToken() public {
// Can't deploy the same token twice
vm.expectRevert(PointTokenVault.PTokenAlreadyDeployed.selector);
Expand Down