Description
The deposit() function overrides the standard ERC4626 deposit() function. According to the ERC4626 standard, when deposit(assets, receiver) is called, the shares should be minted to the receiver address. However, the implementation mints shares to msg.sender instead of receiver, while still recording stakedAsset for the receiver address.
The specific issue is that `_mint(msg.sender, participantShares)` mints shares to the caller instead of the receiver. This breaks the ERC4626 standard interface contract and creates state inconsistency. If user1 deposits on behalf of user2, user1 receives the shares but user2's stake is recorded. This means user2 cannot use the shares to participate in events, and user1 cannot receive the funds if they cancel because the stake is recorded for user2. Also, other users will get the tokens user1 deposited on behalf of user2 when they win.
function deposit(uint256 assets, address receiver) public override returns (uint256) {
require(receiver != address(0));
uint256 stakeAsset = assets - fee;
stakedAsset[receiver] = stakeAsset;
uint256 participantShares = _convertToShares(stakeAsset);
_mint(msg.sender, participantShares);
emit deposited (receiver, stakeAsset);
return participantShares;
}
Risk
**Likelihood**:
* Users may deposit on behalf of other addresses
* The function accepts a `receiver` parameter but doesn't use it for minting
* Breaks expected ERC4626 standard behavior
**Impact**:
* Depositor loses their tokens (cannot join, or withdraw)
* Receiver loses their bet (participates with 0 shares, gets no prize even if country wins)
* Prize pool unfairly distributed to other winners (receiver's portion goes to others)
* Breaks ERC4626 standard interface contract
* May cause integration issues with ERC4626-compatible protocols
Proof of Concept
The following PoC shows how if user1 deposits on behalf of user2 then:
-
user1 can't withdraw the deposited token
-
user1 also can't join an event (noDeposit() error is thrown)
-
user2 can join an event, but they will do it 0 shares, because they were minted to user1
-
user3 gets the prize that would have corresponded to user2, who bet on the winner country with user1's money (but really didn't)
function testSharesMintedToWrongAddress() public {
vm.prank(owner);
briVault.setCountry(countries);
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
uint256 sharesReceived = briVault.deposit(5 ether, user2);
vm.stopPrank();
assertEq(briVault.balanceOf(user1), sharesReceived);
assertEq(briVault.balanceOf(user2), 0);
assertGt(briVault.stakedAsset(user2), 0);
vm.startPrank(user1);
vm.expectRevert(abi.encodeWithSignature("noDeposit()"));
briVault.joinEvent(10);
assertEq(briVault.userSharesToCountry(user1, 10), 0);
console2.log("User1 shares to country 10 after joining event:", briVault.userSharesToCountry(user1, 10));
uint256 balanceBefore = mockToken.balanceOf(user1);
briVault.cancelParticipation();
vm.stopPrank();
assertEq(mockToken.balanceOf(user1), balanceBefore);
vm.startPrank(user2);
briVault.joinEvent(10);
assertEq(briVault.userSharesToCountry(user2, 10), 0);
console2.log("User2 shares to country 10 after joining event:", briVault.userSharesToCountry(user2, 10));
vm.stopPrank();
vm.startPrank(user3);
mockToken.approve(address(briVault), 5 ether);
uint256 user3Shares = briVault.deposit(5 ether, user3);
briVault.joinEvent(10);
vm.stopPrank();
vm.warp(eventEndDate + 1);
vm.startPrank(owner);
briVault.setWinner(10);
uint256 totalWinnerShares = briVault.totalWinnerShares();
vm.stopPrank();
assertEq(totalWinnerShares, user3Shares);
console2.log("User3 shares:", user3Shares);
console2.log("Total winner shares:", totalWinnerShares);
vm.stopPrank();
vm.startPrank(user2);
uint256 balanceBeforeWithdraw = mockToken.balanceOf(user2);
uint256 user2Shares = briVault.balanceOf(user2);
assertEq(user2Shares, 0);
briVault.withdraw();
uint256 balanceAfterWithdraw = mockToken.balanceOf(user2);
assertEq(balanceAfterWithdraw, balanceBeforeWithdraw);
console2.log("User2 bet on the winning couuntry, yet they get :", balanceAfterWithdraw, "assets");
vm.stopPrank();
vm.startPrank(user3);
uint256 balanceBeforeWithdraw3 = mockToken.balanceOf(user3);
briVault.withdraw();
uint256 balanceAfterWithdraw3 = mockToken.balanceOf(user3);
assertGt(balanceAfterWithdraw3, balanceBeforeWithdraw3);
console2.log("User3 received:", balanceAfterWithdraw3 - balanceBeforeWithdraw3, "assets");
vm.stopPrank();
}
Recommended Mitigation
Mint the shares to the `receiver` instead of msg.sender.
function deposit(uint256 assets, address receiver) public override returns (uint256) {
require(receiver != address(0));
// ... existing validation and fee calculation ...
uint256 stakeAsset = assets - fee;
stakedAsset[receiver] = stakeAsset;
uint256 participantShares = _convertToShares(stakeAsset);
IERC20(asset()).safeTransferFrom(msg.sender, participationFeeAddress, fee);
IERC20(asset()).safeTransferFrom(msg.sender, address(this), stakeAsset);
- _mint(msg.sender, participantShares);
+ _mint(receiver, participantShares);
emit deposited (receiver, stakeAsset);
return participantShares;
}
