BriVault
First Flight #52
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: low
Likelihood: low
Invalid

joinEvent() time check is off-by-one (logic inconsistency with deposit() / cancelParticipation())

s3mvl4d

Description

  • All user‑facing actions around the event window should use a consistent boundary rule. If “the event starts at eventStartDate” means “no more deposits or joins at that exact timestamp,” then all functions should enforce the same block.timestamp >= eventStartDate check (or the chosen inclusive/exclusive convention), so users cannot join after deposits are closed.

  • The time check in joinEvent() uses a strict greater‑than (>) comparison, while deposit() and cancelParticipation() use greater‑than‑or‑equal (>=). As a result, joining is still allowed exactly at eventStartDate, but deposits and cancellations are blocked at the same boundary. This off‑by‑one inconsistency enables users to join right at the start (when they should be locked out), creating unfair edge cases and potentially breaking accounting if any last‑moment state changes happen.

// briVault.sol (relevant excerpts)
​
// deposit(): blocks AT start time (>=)
function deposit(uint256 assets, address receiver) public override returns (uint256) {
...
if (block.timestamp >= eventStartDate) { // ← inclusive boundary
revert eventStarted();
}
...
}
​
// cancelParticipation(): blocks AT start time (>=)
function cancelParticipation () public {
if (block.timestamp >= eventStartDate){ // ← inclusive boundary
revert eventStarted();
}
...
}
​
// joinEvent(): blocks only AFTER start time (>)
function joinEvent(uint256 countryId) public {
...
if (block.timestamp > eventStartDate) { // ← exclusive boundary (off-by-one)
revert eventStarted();
}
...
}

Risk

Likelihood: Low

  • At the exact eventStartDate timestamp, UIs and users frequently submit final transactions. Because deposit() and cancelParticipation() revert, users may retry with joinEvent(), which succeeds due to the > check. This will happen naturally at the boundary.

  • Automation / bots can front‑run the boundary to slip in a joinEvent() at eventStartDate while deposits are blocked, taking advantage of the inconsistency.

Impact: Low

  • Policy violation / unfair participation: Participants can change or set their country at the start boundary when deposits and cancellations are already disallowed, violating the intended freeze of choices at event start.

  • Accounting skew: Late joins at the boundary can alter usersAddress, userSharesToCountry, and totals used for winner calculation, creating mismatches with deposit/cancel rules and potentially skewing payouts.

Proof of Concept

  • joinEvent() is allowed at eventStartDate, while deposit() and cancelParticipation() are rejected at the same boundary.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
​
import {Test} from "forge-std/Test.sol";
import {BriVault} from "../src/briVault.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {MockERC20} from "./MockErc20.t.sol";
​
contract JoinTimeOffByOneTest is Test {
BriVault vault;
MockERC20 token;
​
address owner = makeAddr("owner");
address user = makeAddr("user");
address fee = makeAddr("fee");
​
uint256 start;
uint256 end;
string[48] countries;
​
function setUp() public {
start = block.timestamp + 2 days;
end = start + 30 days;
​
token = new MockERC20("Mock", "M");
token.mint(user, 10 ether);
​
vm.startPrank(owner);
vault = new BriVault(IERC20(address(token)), 150, start, fee, 0.0002 ether, end);
countries[10] = "Japan";
vault.setCountry(countries);
vm.stopPrank();
​
vm.startPrank(user);
token.approve(address(vault), type(uint256).max);
vault.deposit(5 ether, user);
vm.stopPrank();
}
​
function test_JoinAllowedAtStartButDepositCancelBlocked() public {
// Move time to EXACTLY eventStartDate
vm.warp(start);
​
// deposit() is blocked at boundary (>=)
vm.startPrank(user);
vm.expectRevert(abi.encodeWithSignature("eventStarted()"));
vault.deposit(1 ether, user);
​
// cancelParticipation() is blocked at boundary (>=)
vm.expectRevert(abi.encodeWithSignature("eventStarted()"));
vault.cancelParticipation();
​
// BUT joinEvent() uses `>` and is ALLOWED at boundary
vault.joinEvent(10); // succeeds due to off-by-one check
​
vm.stopPrank();
}
}

Recommended Mitigation

  • Adopt one consistent boundary convention. If “no actions at the start time” is desired, change joinEvent() to use >= and (optionally) centralize the rule in a modifier.

function joinEvent(uint256 countryId) public {
if (stakedAsset[msg.sender] == 0) { revert noDeposit(); }
if (countryId >= teams.length) { revert invalidCountry(); }
- if (block.timestamp > eventStartDate) {
+ // Align with deposit() and cancelParticipation(): block AT start time
+ 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);
}
Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!