Backd contest - reassor's results

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/access/RoleManager.sol#L156-L157

Vulnerability details

Impact

Revoke role functionality does not remove account from _roleMembers mapping which makes following functions return incorrect data:

• getRoleMember
• getRoleMemberCount

Function renounceGovernance is directly using getRoleMemberCount as a check if it should be possible to remove governance role from the account.

require(getRoleMemberCount(Roles.GOVERNANCE) > 1, Error.CANNOT_REVOKE_ROLE);

Since getRoleMemberCount does not update _roleMembers incorrect data is being returned.

Scenario:

1. There are 2 accounts with governance role
2. Account 1 triggers renounceGovernance
3. Account 2 triggers renounceGovernance
4. Protocol is locked since there are no accounts with governance role

Proof of Concept

• https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/access/RoleManager.sol#L156-L157

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to remove specified account from _roleMembers mapping in _revokeRole internal function.

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/oracles/ChainlinkUsdWrapper.sol#L64 https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/oracles/ChainlinkOracleProvider.sol#L58

Vulnerability details

Impact

Protocol uses Chainlink as an oracle for retrieving prices for the assets.

1. ChainlinkUserWrapper contract in _ethPrice function retrieves ETH price using latestRoundData but the implementation is missing essential security checks that can result in stale and incorrect prices being returned.

2. ChainlinkOracleProvider contract in getPriceUSD is incorrectly checking price by comparing it with 0 value:

require(answer >= 0, Error.NEGATIVE_PRICE);

Instead of >= it should use > to make sure that the price is bigger than 0.

Proof of Concept

• https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/oracles/ChainlinkUsdWrapper.sol#L64
• https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/oracles/ChainlinkOracleProvider.sol#L58

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add checks on the returned data of latestRoundData with proper revert messages if the price is stale or the round is incomplete, for example:

(
roundId,
rawPrice,
,
updateTime,
answeredInRound
) = baseAggregator.latestRoundData();
require(rawPrice > 0, "price <= 0");
require(updateTime != 0, "incomplete round");
require(answeredInRound >= roundId, "stale price");