BriVault
First Flight #52
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Share Transfers Orphan Winners and Lock Vault Assets

galer ah

Root + Impact

Description

Vault shares remain transferable because BriVault inherits ERC20 from ERC4626. Tournament state, however, is keyed by the original depositor: userToCountry and userSharesToCountry are only set inside joinEvent (src/briVault.sol:260-262) and are never updated on transfers. If a participant transfers their shares—or grants an allowance that is pulled—the new holder lacks a team assignment and fails the didNotWin() branch in withdraw() (src/briVault.sol:299-303), while the original depositor owns no shares to redeem.

function joinEvent(uint256 countryId) public {
@> userToCountry[msg.sender] = teams[countryId];
@> userSharesToCountry[msg.sender][countryId] = balanceOf(msg.sender);
}
​
function withdraw() external winnerSet {
if (
keccak256(abi.encodePacked(userToCountry[msg.sender])) !=
@> keccak256(abi.encodePacked(winner))
) revert didNotWin();
...
}

Risk

Likelihood: High – ERC4626 shares are designed to be transferred or pooled; even routine allowance patterns can trigger this.

Impact:

  • Winners who move their shares lose 100% of their stake; the vault retains the funds.

  • Attackers can grief victims by pulling approved shares to another address.

Proof of Concept

  1. User deposits, joins a team, then transfers all shares to another address.

  2. Owner finalizes in favour of that team.

  3. Original depositor calls withdraw() → receives 0 tokens (no shares).

  4. New share holder calls withdraw() → reverts with didNotWin().
    (Demonstrated in test/BriVaultAttack.t.sol:228 through testShareTransferLocksFunds.)

function testShareTransferLocksFunds() public {
briVault.deposit(FIRST_DEPOSIT, winner);
briVault.joinEvent(0);
uint256 shares = briVault.balanceOf(winner);
briVault.transfer(attacker, shares); // transfer after joining
​
vm.prank(owner);
briVault.setWinner(0);
​
vm.prank(attacker);
vm.expectRevert(abi.encodeWithSignature("didNotWin()"));
briVault.withdraw();
}

Recommended Mitigation

  1. Either disable share transfers by overriding _update/_transfer to revert, or

  2. Track team assignments and share snapshots per holder, updating mappings on every transfer/allowance consumption.

  3. Add tests for transfer, transferFrom, and approval flows to ensure accounting remains consistent.

Proposed patch (Solidity-like pseudocode):

function _update(address from, address to, uint256 value) internal override {
+ if (from != address(0) && to != address(0)) revert NonTransferable();
super._update(from, to, value);
}
​
// OR, if transfers must be supported:
function _afterShareChange(address user) internal {
- // currently unused
+ uint256 country = countryOf[user];
+ if (country != INVALID) {
+ userSharesToCountry[user][country] = balanceOf(user);
+ }
}
Updates

Appeal created

bube Lead Judge 16 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Unrestricted ERC4626 functions

Support

FAQs

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

Give us feedback!