Backd contest - rayn's results

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/pool/EthPool.sol#L30 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/vault/EthVault.sol#L29 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/vault/VaultReserve.sol#L81

Vulnerability details

Impact

Transfer ETH by using transfer() may cause this transaction to fail. In EIP-1884:

In many cases, a recipient of ether from a CALL will want to issue a LOG. The LOG operation costs 375 plus 375 per topic. If the LOG also wants to do an SLOAD, this change may make some such transfers fail.

Proof of Concept

contract Vulnerable {
    function withdraw(uint256 amount) external {
        // This forwards 2300 gas, which may not be enough if the recipient
        // is a contract and gas costs change.
        msg.sender.transfer(amount);
    }
}

ref: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Use call{value: amount}() instead of transfer:

      (bool success, ) = payable(msg.sender).call{value: amount}("");
      require(success, "Address: unable to send value, recipient may have reverted");

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/access/RoleManager.sol#L155 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/access/RoleManager.sol#L44 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/access/RoleManager.sol#L127

Vulnerability details

Impact

In RoleManager.sol, _revokeRole() doesn’t remove account from _roleMembers[role]. getRoleMember() and getRoleMemberCount() could then return wrong information. It could result in many problems, e.g., misinformation.

And the most critical problem lies in renounceGovernance(). Since account is not removed from _roleMembers[role] after calling _revokeRole(). getRoleMemberCount() will return the same result after calling renounceGovernance(). Then In renounceGovernance(), getRoleMemberCount(Roles.GOVERNANCE) > 1 is still true even if there is only one governor. In consequence, governors could accidentally remove all the governors of the contract.

Proof of Concept

• Alice and Bob are the governors of the contract.
• Bob want to give up governance, so he calls renounceGovernance(), but his account is not removed from _roleMembers[role]

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/access/RoleManager.sol#L44

    function renounceGovernance() external onlyGovernance {
        require(getRoleMemberCount(Roles.GOVERNANCE) > 1, Error.CANNOT_REVOKE_ROLE);
        _revokeRole(Roles.GOVERNANCE, msg.sender);
    }

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/access/RoleManager.sol#L155

   function _revokeRole(bytes32 role, address account) internal {
        _roles[role].members[account] = false;
        emit RoleRevoked(role, account, msg.sender);
    }

• Alice wants to give up governance too. It should be forbidden because she is the last governor. But getRoleMemberCount(Roles.GOVERNANCE) > 1 is still passed, since _roleMembers[role].length() doesn’t change after bob calling _revokeRole().

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/access/RoleManager.sol#L127

    function getRoleMemberCount(bytes32 role) public view virtual override returns (uint256) {
        if (
            role == Roles.ADDRESS_PROVIDER || role == Roles.POOL_FACTORY || role == Roles.CONTROLLER
        ) {
            return 1;
        }
        if (role == Roles.POOL) {
            return addressProvider.poolsCount();
        }
        if (role == Roles.VAULT) {
            return addressProvider.vaultsCount();
        }
        return _roleMembers[role].length();
    }

• Now there is no governor in this contract.

Tools Used

vim

Recommended Mitigation Steps

account should be removed from _roleMembers[role]

Lines of code

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

Vulnerability details

Impact

Though it checks updatedAt with stalePriceDelay, it still gets an unintended stale price without checking roundId.

Proof of Concept

In getPriceUSD function of ChainlinkOracleProvider.sol:

        (, int256 answer, , uint256 updatedAt, ) = AggregatorV2V3Interface(feed).latestRoundData();

        require(block.timestamp <= updatedAt + stalePriceDelay, Error.STALE_PRICE);
        require(answer >= 0, Error.NEGATIVE_PRICE);

latestRoundData() also returns roundId and answeredInRound, it's not safe to only check updateAt.

Tools Used

vim, ethers.js

Recommended Mitigation Steps

Also check roundId when getting price:

+        (uint80 roundId, int256 answer, , uint256 updatedAt, uint80 answeredInRound) = AggregatorV2V3Interface(feed).latestRoundData();

         require(block.timestamp <= updatedAt + stalePriceDelay, Error.STALE_PRICE);
         require(answer >= 0, Error.NEGATIVE_PRICE);
+        require(answeredInRound >= roundID, Error.STALE_PRICE);

(Non-Critical) It’s better to use SafeERC20

Impact

It’s better to use the trusted 3rd party library SafeERC20 to check the return value of transfer.

Proof of Concept

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/actions/topup/handlers/CompoundHandler.sol#L79-L80

Tools Used

vim, ethers.js

Recommended Mitigation Steps

Use SafeERC20 rather than checking manually.

Save gas in for loops by unchecked arithmetic

The for loop has no overflow risk of i. Use an unchecked block to save gas.

Proof of Concept

access/RoleManager.sol
80:        for (uint256 i = 0; i < roles.length; i++) {

strategies/ConvexStrategyBase.sol
313:        for (uint256 i = 0; i < _rewardTokens.length(); i++) {
380:        for (uint256 i = 0; i < _rewardTokens.length(); i++) {

BkdLocker.sol
310:            for (uint256 i = 0; i < length; i++) {

StakerVault.sol
260:        for (uint256 i = 0; i < actions.length; i++) {

testing/MockStableSwap.sol
30:        for (uint256 i = 0; i < 3; i++) {
42:        for (uint256 i = 0; i < 2; i++) {
70:        for (uint256 i = 0; i < 3; i++) {

Controller.sol
117:        for (uint256 i = 0; i < numActions; i++) {

actions/topup/TopUpAction.sol
188:        for (uint256 i = 0; i < protocols.length; i++) {
456:        for (uint256 i = 0; i < length; i++) {
479:        for (uint256 i = 0; i < length; i++) {
506:        for (uint256 i = 0; i < howMany; i++) {
891:        for (uint256 i = 0; i < length; i++) {

actions/topup/handlers/CTokenRegistry.sol
61:        for (uint256 i = 0; i < ctokens.length; i++) {

actions/topup/handlers/CompoundHandler.sol
135:        for (uint256 i = 0; i < assets.length; i++) {

actions/topup/TopUpKeeperHelper.sol
43:            for (uint256 i = 0; i < users.length; i++) {
46:                for (uint256 j = 0; j < positions.length; j++) {
72:        for (uint256 i = 0; i < keys.length; i++) {
93:        for (uint256 i = 0; i < length; i++) {
165:        for (uint256 i = 0; i < length; i++) {

tokenomics/VestedEscrow.sol
93:        for (uint256 i = 0; i < amounts.length; i++) {

tokenomics/KeeperGauge.sol
155:        for (uint256 i = startEpoch; i < endEpoch; i++) {

tokenomics/InflationManager.sol
91:        for (uint256 i = 0; i < length; i++) {
105:        for (uint256 i = 0; i < length; i++) {
109:        for (uint256 i = 0; i < stakerVaults.length; i++) {
114:        for (uint256 i = 0; i < length; i++) {
166:        for (uint256 i = 0; i < length; i++) {
191:        for (uint256 i = 0; i < length; i++) {
259:        for (uint256 i = 0; i < length; i++) {
283:        for (uint256 i = 0; i < length; i++) {
357:        for (uint256 i = 0; i < length; i++) {
381:        for (uint256 i = 0; i < length; i++) {
404:        for (uint256 i = 0; i < length; i++) {
445:        for (uint256 i = 0; i < length; i++) {

Recommendation

Use unchecked blocks to avoid overflow checks, or use ++i rather than i++ if you don't use unchecked blocks.

for (uint256 i = 0; i < length; ) {
    ...
    unchecked {
        ++i;
    }
}