Olympus DAO contest - PwnPatrol's results

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Heart.sol#L106 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Heart.sol#L112 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/libraries/TransferHelper.sol#L23

Vulnerability details

Impact

If the Heart.sol contract's balance of rewardToken ever falls below the reward amount, the heartbeat will fail and the full protocol's metrics will be thrown off.

Proof of Concept

The beat() function contains a call to _issueReward(msg.sender).

This function uses the TransferHelper library to call rewardToken.safeTransfer(to_, reward);.

In the helper function, the transfer is made using a low level call and fails unless the call returns a success boolean.

(bool success, bytes memory data) = address(token).call(
    abi.encodeWithSelector(ERC20.transfer.selector, to, amount)
);

require(success && (data.length == 0 || abi.decode(data, (bool))), "TRANSFER_FAILED");

As a result, the full beat() function will fail.

Tools Used

VS Code

Recommended Mitigation Steps

Update the _issueReward() function to check the reward amount and the current balance, and send the minimum value out of these two options.

function _issueReward(address to_) internal {
    uint bal = rewardToken.balanceOf(address(this));
    uint toSend = reward > bal ? bal : reward;
    rewardToken.safeTransfer(to_, toSend);
    emit RewardIssued(to_, reward);
}

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/TRSRY.sol#L92

Vulnerability details

Impact

The getLoan() function in the TRSRY.sol contract is a way for users to withdraw approved loans from the protocol, which can be repaid with repayLoan().

No policies have permission to call this function, and it appears it was intended to be publicly callable, but it has the permissioned modifier, so it is functionally useless.

Without the getLoan() function, repayLoan() is functionally useless as well.

Proof of Concept

Checking the permissions set in each of the policies, there is no policy that has permission to call getLoan().

The permissioned modifier requires that the function is only called by policies that have been pre-approved.

/// @notice Modifier to restrict which policies have access to module functions.
modifier permissioned() {
    if (!kernel.modulePermissions(KEYCODE(), Policy(msg.sender), msg.sig))
        revert Module_PolicyNotPermitted(msg.sender);
    _;
}

As a result, this function cannot be called under any circumstances.

Tools Used

VS Code

Recommended Mitigation Steps

Remove the permissioned modifier from the getLoan() function.