Root + Impact
Description
-
In the BriVault::deposit function, there is a receiver argument which is used for storing the staked amount in the stakedAsset mapping. Meanwhile the asset amount for deposit and fee is deducted from msg.sender's account and the share token is also minted to it.
-
If the user enters an address different than the one they are calling the function from (i.e. msg.sender), then, the required information will be splitted and partially stored for various accounts. This will prevent the user to call the BriVault::joinEvent function and participate in the event. They will not even be able to call the BriVault::cancelParticipation function and get a refund.
function deposit(uint256 assets, address receiver) public override returns (uint256) {
require(receiver != address(0));
if (block.timestamp >= eventStartDate) {
revert eventStarted();
}
uint256 fee = _getParticipationFee(assets);
if (minimumAmount + fee > assets) {
revert lowFeeAndAmount();
}
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);
@> emit deposited (receiver, stakeAsset);
return participantShares;
}
function joinEvent(uint256 countryId) public {
@> if (stakedAsset[msg.sender] == 0) {
@> revert noDeposit();
}
if (countryId >= teams.length) {
revert invalidCountry();
}
if (block.timestamp > eventStartDate) {
revert eventStarted();
}
userToCountry[msg.sender] = teams[countryId];
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
usersAddress.push(msg.sender);
numberOfParticipants++;
totalParticipantShares += participantShares;
emit joinedEvent(msg.sender, countryId);
}
function cancelParticipation() public {
if (block.timestamp >= eventStartDate){
revert eventStarted();
}
@> uint256 refundAmount = stakedAsset[msg.sender];
stakedAsset[msg.sender] = 0;
uint256 shares = balanceOf(msg.sender);
_burn(msg.sender, shares);
@> IERC20(asset()).safeTransfer(msg.sender, refundAmount);
}
Risk
Likelihood: Medium
Impact: High
-
When it happens the user will not be able to particiapte in the event or refund.
-
The second impact is that the fund will remain in the vault, while its balance is supposed to be 0 after all claims.
Proof of Concept
Please copy/paste the following function into the test file, and run it.
function test_userCannotParticipateOrRefundWhenReceiverDifferentFromMsgSender()
public
{
address receiver = makeAddr("Another User");
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, receiver);
vm.expectRevert(abi.encodeWithSignature("noDeposit()"));
briVault.joinEvent(20);
uint256 user1BalanceBefore = IERC20(address(mockToken)).balanceOf(
user1
);
briVault.cancelParticipation();
vm.stopPrank();
uint256 user1BalanceAfter = IERC20(address(mockToken)).balanceOf(user1);
assert(user1BalanceAfter == user1BalanceBefore);
}
Recommended Mitigation
To solve this issue we need to remove the receiver parameter from the function declaration and replace its instances in the body of the function with msg.sender.
- function deposit(uint256 assets, address receiver) public override returns (uint256) {
+ function deposit(uint256 assets) public override returns (uint256) {
- require(receiver != address(0));
+ require(msg.sender != address(0));
if (block.timestamp >= eventStartDate) {
revert eventStarted();
}
uint256 fee = _getParticipationFee(assets);
// charge on a percentage basis points
if (minimumAmount + fee > assets) {
revert lowFeeAndAmount();
}
uint256 stakeAsset = assets - fee;
- stakedAsset[receiver] = stakeAsset;
+ stakedAsset[msg.sender] = stakeAsset;
uint256 participantShares = _convertToShares(stakeAsset);
IERC20(asset()).safeTransferFrom(msg.sender, participationFeeAddress, fee);
IERC20(asset()).safeTransferFrom(msg.sender, address(this), stakeAsset);
_mint(msg.sender, participantShares);
- emit deposited (receiver, stakeAsset);
+ emit deposited (msg.sender, stakeAsset);
return participantShares;
}
