KYC parameters can be changed in the KYC variants of the Cash token and lock users out of their funds

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/token/CashKYCSender.sol#L34 https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/token/CashKYCSender.sol#L40 https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/token/CashKYCSenderReceiver.sol#L34 https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/token/CashKYCSenderReceiver.sol#L40

Vulnerability details

The CashKYCSender and CashKYCSenderReceiver variants of the CASH token restrict transfers to senders or receivers that have a valid KYC status according to the KYC registry and the requirement group.

However, these two parameters are mutable and can be changed at any time by a privileged account with the corresponding access role.

Impact

Any change in the KYC registry or the KYC requirement group will leave all current users that have a valid KYC status in the current registry and group locked out of their funds.

Since KYC statuses are tied to a particular registry and group, changing any of these two parameters will result in transfers being reverted due to the constraints placed in the _beforeTokenTransfer hook.

PoC

The following test illustrates the issue using the CashKYCSender token.

contract TestAudit is BasicDeployment {
    function test_CashKYCSender_ChangeRegistry() public {
        createDeploymentCashKYCSender();

        // Add alice to current KYC registry
        _addAddressToKYC(kycRequirementGroup, alice);

        // Provide alice with CASH tokens
        _addAddressToKYC(kycRequirementGroup, guardian);
        vm.prank(guardian);
        cashKYCSenderProxied.mint(alice, 100e18);

        // Alice can transfer tokens
        vm.prank(alice);
        cashKYCSenderProxied.transfer(address(1), 1e18);

        // Deploy a new registry and change it in the token
        deployKYCRegistry();
        vm.startPrank(guardian);
        cashKYCSenderProxied.grantRole(cashKYCSenderProxied.KYC_CONFIGURER_ROLE(), guardian);
        cashKYCSenderProxied.setKYCRegistry(address(registry));
        vm.stopPrank();

        // Alice can't transfer anymore
        vm.prank(alice);
        vm.expectRevert();
        cashKYCSenderProxied.transfer(address(1), 1e18);
    }
}

Recommendation

Remove the mutability of the KYC registry and KYC requirement group variables, and make them immutable by removing the setters.

Particular users can still be removed from the registry or changed from group if needed.

[c4-judge]

Duplicate of https://github.com/code-423n4/2023-01-ondo-findings/issues/330

[c4-judge]

trust1995 marked the issue as grade-b