code-423n4 / 2021-04-maple-findings

0 stars 0 forks source link

Incorrect input validation on the most critical admin role in the protocol in MapleGlobals.sol #37

Closed code423n4 closed 3 years ago

code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

The global admin defined in MapleGlobals is the most critical admin in the protocol as indicated in the comment on L20 of MapleGlobals.sol: “Admin of the whole network, has the power to switch off/on the functionality of entire protocol.”

This admin address is set on L89 of the constructor without any input validation. But more importantly, the setAdmin function which is callable only by the protocol Governor has an incorrect input validation on L151:

require(msg.sender == governor && admin != address(0), "MapleGlobals:UNAUTHORIZED");

where, instead of checking the parameter newAdmin against zero address, the require statement checks the existing admin variable against zero address.

This incorrect check allows newAdmin to be a zero address which is then assigned to admin. This flow will prevent the pausing/unpausing of the protocol because of the check on L183 in setProtocolPause. Furthermore, this one time mistake of setting the admin to zero-address cannot be fixed (without contract redeployment) because of the incorrect check on L151 (indicated above) which will now trigger for zero-address of admin.

Proof of Concept

admin variable: https://github.com/maple-labs/maple-core/blob/355141befa89c7623150a83b7d56a5f5820819e9/contracts/MapleGlobals.sol#L20

Incorrect input validation: https://github.com/maple-labs/maple-core/blob/355141befa89c7623150a83b7d56a5f5820819e9/contracts/MapleGlobals.sol#L153

Use of admin in pausing/unpausing: https://github.com/maple-labs/maple-core/blob/355141befa89c7623150a83b7d56a5f5820819e9/contracts/MapleGlobals.sol#L185

Tools Used

Manual Analysis

Recommended Mitigation Steps

Change admin to newAdmin on L151: require(msg.sender == governor && newAdmin != address(0), "MapleGlobals:UNAUTHORIZED");

lucas-manuel commented 3 years ago

This is not a bug, this was intended. If the address is set to zero, it is an intentional revoke of the permission.