The Restriction Manager does not completely implement ERC1404 which leads to account that are supposed to be restricted actually have access to do with their tokens as they see fit

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/token/RestrictionManager.sol#L28-L34

Vulnerability details

Impact

Medium, contract's intended logic is for blacklisted users not to be able to interact with their system so as to follow rules set by regulationary bodies in the case where a user does anything that warrants them to be blacklisted, but this is clearly broken since only half the window is closed as current implementation only checks on receiver being blacklisted and not sender.

Proof of Concept

The current implementation of the ERC1404 restrictions within the RestrictionManager.sol contract only places restrictions on the receiving address in token transfer instances. This oversight means that the sending addresses are not restricted, which poses a regulatory and compliance risk. Should a user be blacklisted for any reason, they can continue to transfer tokens as long as the receiving address is a valid member. This behaviour is contrary to expectations from regulatory bodies, especially say in the U.S where these bodies are very strict and a little in-compliance could land Centrifuge a lawsuit., which may expect complete trading restrictions for such blacklisted individuals.

Within the RestrictionManager contract, the method detectTransferRestriction only checks if the receiving address (to) is a valid member:

RestrictionManager.sol#L28-L34

function detectTransferRestriction(address from, address to, uint256 value) public view returns (uint8) {
    if (!hasMember(to)) {
        return DESTINATION_NOT_A_MEMBER_RESTRICTION_CODE;
    }
    return SUCCESS_CODE;
}

In the above code, the sending address (from) is never checked against the membership restrictions, which means blacklisted users can still initiate transfers and when checking the transfer restriction from both tranchtoken.sol and the liquiditypool.sol it's going to wrongly return true for a personnel that should be false

See Tranche.sol#L80-L82)

// function checkTransferRestriction(address from, address to, uint256 value) public view returns (bool) {
//     return share.checkTransferRestriction(from, to, value);
// }

Also Tranche.sol#L35-L39

    modifier restricted(address from, address to, uint256 value) {
        uint8 restrictionCode = detectTransferRestriction(from, to, value);
        require(restrictionCode == restrictionManager.SUCCESS_CODE(), messageForTransferRestriction(restrictionCode));
        _;
    }

This function suggests that the system's logic may rely on the detectTransferRestriction method in other parts of the ecosystem. Consequently, if the restriction manager's logic is flawed, these other parts may also allow unauthorised transfers.

Foundry POC

Add this to the Tranche.t.sol contract

    function testTransferFromTokensFromBlacklistedAccountWorks(uint256 amount, address targetUser, uint256 validUntil) public {
        vm.assume(baseAssumptions(validUntil, targetUser));

        restrictionManager.updateMember(targetUser, validUntil);
        assertEq(restrictionManager.members(targetUser), validUntil);
        restrictionManager.updateMember(address(this), block.timestamp);
        assertEq(restrictionManager.members(address(this)), block.timestamp);

        token.mint(address(this), amount);
        vm.warp(block.timestamp + 1);

        token.transferFrom(address(this), targetUser, amount);
        assertEq(token.balanceOf(targetUser), amount);
    }

As seen even after address(this) stops being a member they could still transfer tokens to another user in as much as said user is still a member, which means a blacklisted user could easily do anything with their tokens all they need to do is to delegate to another member.

Tool used

Manual Review

Recommended Mitigation Steps

RefactordetectTransferRestriction(), i.e modify the method to also validate the sending address (from). Ensure both from and to addresses are valid members before allowing transfers.

If this is contract's intended logic then this information should be duly passed to the regulatory bodies that a user can't really get blacklisted and more of user's could be stopped from receiving tokens.

Assessed type

Context

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #233

[c4-judge]

gzeon-c4 marked the issue as not a duplicate

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[hieronx]

This is a design decision. Transferring to a valid member is fine if that destination is still allowed to hold the tokens.

[gzeon-c4]

This is a design decision. Transferring to a valid member is fine if that destination is still allowed to hold the tokens.

This seems to contradict with Removing an investor from the memberlist in the Restriction Manager locks their tokens. This is expected behaviour. https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/README.md?plain=1#L140

[c4-judge]

gzeon-c4 removed the grade

[hieronx]

Yes you're right, that's unfortunate wording. What I said above is correct, and the text in the README is incorrect. What was meant was that it locks their tranche tokens from being redeemed.

It is indeed fair to say though that based on the README text, the above issue is valid.

[c4-judge]

gzeon-c4 marked the issue as primary issue

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 marked the issue as selected for report

[hieronx]

Mitigated in centrifuge/liquidity-pools#138