Improper Access Control

[code423n4]

Handle

defsec

Vulnerability details

Impact

The software does not restrict or incorrectly restricts access to a resource from an unauthorized actor. During the observation of https://github.com/code-423n4/2021-10-tempus/blob/main/contracts/TempusPool.sol contract, on the code access control is misconfigured.

Proof of Concept

1. Navigate to "https://github.com/code-423n4/2021-10-tempus/blob/main/contracts/TempusPool.sol#L137" contract.
2. The sample code can be seen from the Line #117.

 function transferFees(address authorizer, address recipient) external override onlyController {
        require(authorizer == owner(), "Only owner can transfer fees.");
        uint256 amount = totalFees;
        totalFees = 0;

        IERC20 token = IERC20(yieldBearingToken);
        token.safeTransfer(recipient, amount);
    }

3. Authorizer address is compared with owner address. However any controller can bypass this protection with sending valid authorizer address.

Tools Used

Code Review

Recommended Mitigation Steps

Consider to replace onlyController modifier with onlyOwner modifier. If you want to double check access control, implement necessary permission like onlyOwnerAndController modifier.

[mijovic]

While this is a fair point, I think there is no particular risk for this one. The controller is set during deployment and can't be changed in any way. If you look at TempusController.transferFees it passes msg.sender as authorizer, and transferFees in TempusPool are onlyController. If there is a malicious controller this would be clearly visible after deployment.

The solution for this can be to make this owner only, but then we are exposed to possible reentrancy from the token transfer method, which is in this case untrusted token. In that case, we would need to implement reentrancy protection for TempusPool, so I would not go that way (reentrancy protection is in TempusController in all places where it was needed).

Part with onlyOwnerController I don't really understand what that would be.

All in all, I would say this is not a security risk for us.

[0xean]

I am not sure I understand the re-entrancy concern here -

    function transferFees(address authorizer, address recipient) external override onlyController {
        require(authorizer == owner(), "Only owner can transfer fees.");
        uint256 amount = totalFees;
        totalFees = 0;

        IERC20 token = IERC20(yieldBearingToken);
        token.safeTransfer(recipient, amount);
    }

This function does not have a vulnerability for a re-entrancy attack based on the pattern implemented. A second call would end up transferring 0 total fees.

While I do follow the logic that has been implemented, I think the access control pattern is a bit confusing and the require statement make this function only "secure" because of how its currently being called which is probably a bit of an anti-pattern.

I don't see the current attack vector with how things are written / being called so will downgrade the severity to Non-Critical as I believe there is a clearer implementation that will probably be safer if these contracts are ever modified or updated.