Users from excluded countries can stake SALT by claiming airdrops

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/launch/Airdrop.sol#L74-L8

Vulnerability details

Impact

The protocol implements checks to avoid users from excluded countries to stake assets, but it is possible to do so by claiming airdrops.

Vulnerability Details

Users from excluded countries have restrictions on deposits, as explained on the AccessManager:

The AccessManager restricts users from adding liquidity, adding collateral and borrowing USDS on the contract level

Staking SALT is also restricted:

function stakeSALT( uint256 amountToStake ) external nonReentrant {
    require( exchangeConfig.walletHasAccess(msg.sender), "Sender does not have exchange access" );

Staking.sol#L41-L43

This test also shows the intention to prevent claiming airdrops (and staking SALT) for wallets without exchange access (excluded countries):

... verifies that the address was eligible to claim the airdrop(... and the wallet has exchange access)

The problem is that there is no restriction for users from excluded countries to claim the airdrops.

This is proved on the following POC.

Proof of Concept

1. Copy this test to src/launch/tests/Airdrop.t.sol
2. Run COVERAGE="no" NETWORK="sep" forge test -vv --rpc-url https://1rpc.io/sepolia --mt "testClaimAirdropExcludedCountries"

function testClaimAirdropExcludedCountries() external {
    bytes32 SALT_POOL_ID = bytes32(0);

    // Whitelist Alice for the Airdrop
    whitelistAlice();

    // Increment the `geohash` a few times to make sure that no signature is valid for any country
    vm.startPrank(address(dao));
    accessManager.excludedCountriesUpdated();
    accessManager.excludedCountriesUpdated();
    accessManager.excludedCountriesUpdated();
    vm.stopPrank();

    // Approve the distribution to allow claiming
    vm.prank(address(bootstrapBallot));
    initialDistribution.distributionApproved();
    assertTrue(airdrop.claimingAllowed(), "Claiming should be allowed for the test");

    // Make sure alice has not claimed yet
    assertFalse(airdrop.claimed(alice), "Alice should not have claimed for the test");

    // Make sure Alice doesn't have any shares in the SALT pool
    assertEq(staking.userShareForPool(alice, SALT_POOL_ID), 0);

    // Make sure that Alice DOESN'T have wallet access
    // Meaning that she wasn't granted access like in excluded countries
    assertEq(accessManager.walletHasAccess(alice), false);

    // Alice can still claim the airdrop
    vm.prank(alice);
    airdrop.claimAirdrop();

    // Verify that Alice successfully claimed
    assertTrue(airdrop.claimed(alice), "Alice should have successfully claimed the airdrop");

    // Verify that Alice successfully staked SALT via the airdrop
    assertEq(staking.userShareForPool(alice, SALT_POOL_ID), 5000000 ether);
}

Recommended Mitigation Steps

One way to achieve this is to add a check to the claimAirdrop function. Another option is to prevent those users from voting altogether (but if the country is excluded later, they will still be able to claim + stake SALT).

    function claimAirdrop() external nonReentrant {
+       require(exchangeConfig.walletHasAccess(msg.sender), "Sender does not have exchange access");
        require(claimingAllowed, "Claiming is not allowed yet");
        require(isAuthorized(msg.sender), "Wallet is not authorized for airdrop");
        require(!claimed[msg.sender], "Wallet already claimed the airdrop");

        // Have the Airdrop contract stake a specified amount of SALT and then transfer it to the user
        staking.stakeSALT(saltAmountForEachUser);
        staking.transferStakedSaltFromAirdropToUser(msg.sender, saltAmountForEachUser);

        claimed[msg.sender] = true;
    }

Assessed type

Other

[c4-judge]

Picodes marked the issue as duplicate of #964

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)