Alphabet Case inconsitency can break the proposeCountryExclusion(...) and proposeCountryInclusion(...) functionality - See Coded POC

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L233 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L224

Vulnerability details

Impact

This can lead to false negatives, e.g same country can be included and excluded at the same time depending on the alphabet case used during the proposal creation. This will break the functionality of the protocol.

Vulnerability Details

This vulnerability stems from the check used to validated the country a proposal is being created for when

Proposals.proposeCountryInclusion(...) and Proposals.proposeCountryExclusion(...) are called. The developer assumes that checking the byte length of the ISO 3166 Alpha-2 Code is sufficient to ensure no other character is entered as shown below:

require( bytes(country).length == 2, "Country must be an ISO 3166 Alpha-2 Code" );

However, as shown in the coded POC below, different case combinations of country ISO 3166 Alpha-2 Code will pass this check. Leading to situations like:

Pakistan - which can be PK, pK, pk, Pk will all pass this test because they all resolve to 2 bytes in length

If the proposal was made for pk to be excluded, then

DAO.countryIsExcluded(pk) will be true while DAO.countryIsExcluded(PK) will be false meaning one country can be both included and excluded at the same time which breaks the protocols functionality.

Coded POC

Add the following test case to the DAO.t.sol file and run COVERAGE="yes" NETWORK="sep" forge test --mt testExcludeCountryApproved -vv --rpc-url https://rpc.ankr.com/eth_sepolia

I also added the bytes output of other combinations to make this clearer. You can as well run the test for other country code combinations.

function testExcludeCountryApproved() public
        {
        vm.startPrank(alice);
        staking.stakeSALT( 1000000 ether );

        proposals.proposeCountryExclusion( "jp", "exclude Japan" );
        _voteForAndFinalizeBallot(1, Vote.YES);

        assertTrue( dao.countryIsExcluded( "jp" ), "jp (Japan) should be excluded" );
        assertFalse(dao.countryIsExcluded( "JP" ), "JP (Japan) should not be excluded");

        // @audit output of different combination of ISO 3166 Alpha-2 Code that would pass for a proposal and break protocol functionality
        emit log_named_bytes("Bytes of jP: ", bytes("jP"));
        emit log_named_bytes("Bytes of JP: ", bytes("JP"));
        emit log_named_bytes("Bytes of ZZ: ", bytes("ZZ"));
        emit log_named_bytes("Bytes of zz: ", bytes("zz"));
        emit log_named_bytes("Bytes of US: ", bytes("US"));
        emit log_named_bytes("Bytes of us: ", bytes("us"));
        emit log_named_bytes("Bytes of PK: ", bytes("PK"));
        emit log_named_bytes("Bytes of pk: ", bytes("pk"));

        }

Result of logs:

Logs:
  Bytes of jP: : 0x6a50
  Bytes of JP: : 0x4a50
  Bytes of ZZ: : 0x5a5a
  Bytes of zz: : 0x7a7a
  Bytes of US: : 0x5553
  Bytes of us: : 0x7573
  Bytes of PK: : 0x504b
  Bytes of pk: : 0x706b

SUGGESTION

The only solution I could suggest at this time is:

on the frontend of the protocol, implement conversion of the user input to upper case OR attempt converting from user entered case to UPPER case in all instances.

Assessed type

Invalid Validation

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) acknowledged

[Picodes]

This would be a configuration mistake by the admin or the DAO so falls within QA

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)